| Message ID | 20210319030128.1345061-3-yanaijie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: check the whole result in some places | expand |
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. Doug? Cheers, Hannes
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
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; }
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(-)