Re: [PATCH 2/3] scsi: only copy data to user when the whole result is good

[Jason Yan <yanaijie@huawei.com>]

Hi Hannes,

在 2021/3/19 15:56, Hannes Reinecke 写道:
> On 3/19/21 4:01 AM, Jason Yan wrote:
>> When the scsi device status is offline, mode sense command will return a
>> result with only DID_NO_CONNECT set. Then in sg_scsi_ioctl(),
>> only status byte of the result is checked, and because of
>> bug [1], garbage data is copied to the userspace.
>>
>> Only copy the buffer to userspace when the whole result is good.
>>
>> [1] 
>> https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/ 
>>
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   block/scsi_ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
>> index 6599bac0a78c..359bf0003af4 100644
>> --- a/block/scsi_ioctl.c
>> +++ b/block/scsi_ioctl.c
>> @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct 
>> gendisk *disk, fmode_t mode,
>>               if (copy_to_user(sic->data, req->sense, bytes))
>>                   err = -EFAULT;
>>           }
>> -    } else {
>> +    } else if (scsi_result_is_good(req->result)) {
>>           if (copy_to_user(sic->data, buffer, out_len))
>>               err = -EFAULT;
>>       }
>>
> Hmm. Not sure about this one.
> The prime motivator behind sg is to get _precisely_ all flags of the 
> command, and not do in-kernel error handling.
> So one could argue that this behaviour is intentional, and would break 
> existing use-cases.
> 

Thanks for the review.

The existing usersapce can do nothing with the uninitialized data. Or 
the driver or disk may fill some data and at the same time set host_byte 
or driver_byte to non-zero? I'm not sure about this. And the return 
value of sg_scsi_ioctl() just get the status byte(only 8 bit), how can 
the users know about this situation?

Thanks,
Jason

> Doug?
> 
> Cheers,
> 
> Hannes