[PATCH] scsi: sr: Fix a potential uninit-value in sr_get_events()

[PATCH] scsi: sr: Fix a potential uninit-value in sr_get_events()

[Shigeru Yoshida]

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(-)

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

Re: [PATCH] scsi: sr: Fix a potential uninit-value in sr_get_events()

[Bart Van Assche]

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.

Re: [PATCH] scsi: sr: Fix a potential uninit-value in sr_get_events()

[Shigeru Yoshida]

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.
> 

Re: [PATCH] scsi: sr: Fix a potential uninit-value in sr_get_events()

[michael.christie]

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