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 <[email protected]>
---
drivers/scsi/sr.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
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)
--
2.39.0
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.
Thanks,
Bart.
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