| Message ID | 20230531164346.118438-1-syoshida@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: sr: Fix a potential uninit-value in sr_get_events() | expand |
On Wed, 31 May 2023 15:44:17 -0700, Bart Van Assche wrote: > On 5/31/23 09:43, Shigeru Yoshida wrote: >> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c >> index 12869e6d4ebd..86b43c069a44 100644 >> --- a/drivers/scsi/sr.c >> +++ b/drivers/scsi/sr.c >> @@ -177,10 +177,13 @@ static unsigned int sr_get_events(struct >> scsi_device *sdev) >> result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buf, sizeof(buf), >> SR_TIMEOUT, MAX_RETRIES, &exec_args); >> + if (result) >> + return 0; >> + >> if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION) >> return DISK_EVENT_MEDIA_CHANGE; >> - if (result || be16_to_cpu(eh->data_len) < sizeof(*med)) >> + if (be16_to_cpu(eh->data_len) < sizeof(*med)) >> return 0; > > I think this change is wrong because it introduces an unintended > behavior > change. A better solution is probably to zero-initialize sshdr before > scsi_execute_cmd() is called. Hi Bart, I'm very sorry for the very late response, and thank you so much for your feedback. I'll prepare the v2 patch as you suggested. Thanks, Shigeru > > Thanks, > > Bart. >
On 10/30/23 3:13 AM, Shigeru Yoshida wrote: > On Wed, 31 May 2023 15:44:17 -0700, Bart Van Assche wrote: >> On 5/31/23 09:43, Shigeru Yoshida wrote: >>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c >>> index 12869e6d4ebd..86b43c069a44 100644 >>> --- a/drivers/scsi/sr.c >>> +++ b/drivers/scsi/sr.c >>> @@ -177,10 +177,13 @@ static unsigned int sr_get_events(struct >>> scsi_device *sdev) >>> result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buf, sizeof(buf), >>> SR_TIMEOUT, MAX_RETRIES, &exec_args); >>> + if (result) >>> + return 0; >>> + >>> if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION) >>> return DISK_EVENT_MEDIA_CHANGE; >>> - if (result || be16_to_cpu(eh->data_len) < sizeof(*med)) >>> + if (be16_to_cpu(eh->data_len) < sizeof(*med)) >>> return 0; >> >> I think this change is wrong because it introduces an unintended >> behavior >> change. A better solution is probably to zero-initialize sshdr before >> scsi_execute_cmd() is called. > > Hi Bart, > > I'm very sorry for the very late response, and thank you so much for > your feedback. I'll prepare the v2 patch as you suggested. We actually just did this patch for the next kernel where we just check result before accessing the sshdr: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=6.7/scsi-staging&id=f7d7129c6c24168b9be7709b0b37767b5f743cf3
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 12869e6d4ebd..86b43c069a44 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -177,10 +177,13 @@ static unsigned int sr_get_events(struct scsi_device *sdev) result = scsi_execute_cmd(sdev, cmd, REQ_OP_DRV_IN, buf, sizeof(buf), SR_TIMEOUT, MAX_RETRIES, &exec_args); + if (result) + return 0; + if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION) return DISK_EVENT_MEDIA_CHANGE; - if (result || be16_to_cpu(eh->data_len) < sizeof(*med)) + if (be16_to_cpu(eh->data_len) < sizeof(*med)) return 0; if (eh->nea || eh->notification_class != 0x4)
KMSAN found the following issue: BUG: KMSAN: uninit-value in sr_check_events+0x365/0x1460 sr_check_events+0x365/0x1460 cdrom_check_events+0x66/0x170 sr_block_check_events+0xf2/0x130 disk_check_events+0xec/0x900 bdev_check_media_change+0x2a6/0x7d0 sr_block_open+0x154/0x320 blkdev_get_whole+0xa8/0x6c0 blkdev_get_by_dev+0x50f/0x1200 blkdev_open+0x215/0x430 do_dentry_open+0xfbd/0x19a0 vfs_open+0x7b/0xa0 path_openat+0x4a54/0x5b40 do_filp_open+0x24d/0x660 do_sys_openat2+0x1f0/0x910 __x64_sys_openat+0x2b4/0x330 do_syscall_64+0x41/0xc0 entry_SYSCALL_64_after_hwframe+0x63/0xcd Local variable sshdr.i created at: sr_check_events+0x131/0x1460 cdrom_check_events+0x66/0x170 sr_get_events() can access uninitialized local variable sshdr when scsi_execute_cmd() returns error. This patch fixes the issue by checking the result before accessing sshdr. Fixes: 93aae17af117 ("sr: implement sr_check_events()") Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> --- drivers/scsi/sr.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)