* [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
@ 2019-10-12 3:25 zhengbin
2019-10-17 0:05 ` Finn Thain
2019-10-17 2:45 ` Bart Van Assche
0 siblings, 2 replies; 5+ messages in thread
From: zhengbin @ 2019-10-12 3:25 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi; +Cc: yi.zhang, zhengbin13
kmsan report a warning in 5.1-rc4:
BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline]
BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: G B 5.1.0-rc4+ #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x173/0x1d0 lib/dump_stack.c:113
kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619
__msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310
sr_get_events drivers/scsi/sr.c:207 [inline]
sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
The reason is as follows:
sr_get_events
struct scsi_sense_hdr sshdr; -->uninit
scsi_execute_req -->If fail, will not set sshdr
scsi_sense_valid(&sshdr) -->access sshdr
We can init sshdr in sr_get_events, but there have many callers of
scsi_execute, scsi_execute_req, we have to troubleshoot all callers,
the simpler way is init sshdr in __scsi_execute.
BTW: sr_do_ioctl should check whether sshdr is valid, fix this
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
v1->v2: modify the comments suggested by Bart
v2->v3: fix bug in sr_do_ioctl
drivers/scsi/scsi_lib.c | 7 +++++++
drivers/scsi/sr_ioctl.c | 5 +++++
2 files changed, 12 insertions(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5447738..d5e29c5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
struct scsi_request *rq;
int ret = DRIVER_ERROR << 24;
+ /*
+ * Zero-initialize sshdr for those callers that check the *sshdr
+ * contents even if no sense data is available.
+ */
+ if (sshdr)
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
req = blk_get_request(sdev->request_queue,
data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index ffcf902..335cfdd 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
/* Minimal error checking. Ignore cases we know about, and report the rest. */
if (driver_byte(result) != 0) {
+ if (!scsi_sense_valid(sshdr)) {
+ err = -EIO;
+ goto out;
+ }
+
switch (sshdr->sense_key) {
case UNIT_ATTENTION:
SDev->changed = 1;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
2019-10-12 3:25 [PATCH v3] scsi: core: fix uninit-value access of variable sshdr zhengbin
@ 2019-10-17 0:05 ` Finn Thain
2019-10-17 1:16 ` zhengbin (A)
2019-10-17 2:45 ` Bart Van Assche
1 sibling, 1 reply; 5+ messages in thread
From: Finn Thain @ 2019-10-17 0:05 UTC (permalink / raw)
To: zhengbin; +Cc: bvanassche, jejb, martin.petersen, linux-scsi, yi.zhang
On Sat, 12 Oct 2019, zhengbin wrote:
> kmsan report a warning in 5.1-rc4:
>
> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline]
> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: G B 5.1.0-rc4+ #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x173/0x1d0 lib/dump_stack.c:113
> kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619
> __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310
> sr_get_events drivers/scsi/sr.c:207 [inline]
> sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
>
> The reason is as follows:
> sr_get_events
> struct scsi_sense_hdr sshdr; -->uninit
> scsi_execute_req -->If fail, will not set sshdr
> scsi_sense_valid(&sshdr) -->access sshdr
>
> We can init sshdr in sr_get_events, but there have many callers of
> scsi_execute, scsi_execute_req, we have to troubleshoot all callers,
> the simpler way is init sshdr in __scsi_execute.
>
> BTW: sr_do_ioctl should check whether sshdr is valid, fix this
>
> Signed-off-by: zhengbin <zhengbin13@huawei.com>
> ---
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> drivers/scsi/scsi_lib.c | 7 +++++++
> drivers/scsi/sr_ioctl.c | 5 +++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5447738..d5e29c5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> struct scsi_request *rq;
> int ret = DRIVER_ERROR << 24;
>
> + /*
> + * Zero-initialize sshdr for those callers that check the *sshdr
> + * contents even if no sense data is available.
> + */
> + if (sshdr)
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
> req = blk_get_request(sdev->request_queue,
> data_direction == DMA_TO_DEVICE ?
> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
Does this have any effect? It appears __scsi_execute() calls
scsi_normalize_sense(), which already does
memset(sshdr, 0, sizeof(struct scsi_sense_hdr)).
--
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index ffcf902..335cfdd 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>
> /* Minimal error checking. Ignore cases we know about, and report the rest. */
> if (driver_byte(result) != 0) {
> + if (!scsi_sense_valid(sshdr)) {
> + err = -EIO;
> + goto out;
> + }
> +
> switch (sshdr->sense_key) {
> case UNIT_ATTENTION:
> SDev->changed = 1;
> --
> 2.7.4
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
2019-10-17 0:05 ` Finn Thain
@ 2019-10-17 1:16 ` zhengbin (A)
0 siblings, 0 replies; 5+ messages in thread
From: zhengbin (A) @ 2019-10-17 1:16 UTC (permalink / raw)
To: Finn Thain; +Cc: bvanassche, jejb, martin.petersen, linux-scsi, yi.zhang
On 2019/10/17 8:05, Finn Thain wrote:
> On Sat, 12 Oct 2019, zhengbin wrote:
>
>> kmsan report a warning in 5.1-rc4:
>>
>> BUG: KMSAN: uninit-value in sr_get_events drivers/scsi/sr.c:207 [inline]
>> BUG: KMSAN: uninit-value in sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
>> CPU: 1 PID: 13858 Comm: syz-executor.0 Tainted: G B 5.1.0-rc4+ #8
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x173/0x1d0 lib/dump_stack.c:113
>> kmsan_report+0x131/0x2a0 mm/kmsan/kmsan.c:619
>> __msan_warning+0x7a/0xf0 mm/kmsan/kmsan_instr.c:310
>> sr_get_events drivers/scsi/sr.c:207 [inline]
>> sr_check_events+0x2cf/0x1090 drivers/scsi/sr.c:243
>>
>> The reason is as follows:
>> sr_get_events
>> struct scsi_sense_hdr sshdr; -->uninit
>> scsi_execute_req -->If fail, will not set sshdr
>> scsi_sense_valid(&sshdr) -->access sshdr
>>
>> We can init sshdr in sr_get_events, but there have many callers of
>> scsi_execute, scsi_execute_req, we have to troubleshoot all callers,
>> the simpler way is init sshdr in __scsi_execute.
>>
>> BTW: sr_do_ioctl should check whether sshdr is valid, fix this
>>
>> Signed-off-by: zhengbin <zhengbin13@huawei.com>
>> ---
>> v1->v2: modify the comments suggested by Bart
>> v2->v3: fix bug in sr_do_ioctl
>> drivers/scsi/scsi_lib.c | 7 +++++++
>> drivers/scsi/sr_ioctl.c | 5 +++++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 5447738..d5e29c5 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>> struct scsi_request *rq;
>> int ret = DRIVER_ERROR << 24;
>>
>> + /*
>> + * Zero-initialize sshdr for those callers that check the *sshdr
>> + * contents even if no sense data is available.
>> + */
>> + if (sshdr)
>> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
>> +
>> req = blk_get_request(sdev->request_queue,
>> data_direction == DMA_TO_DEVICE ?
>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> Does this have any effect? It appears __scsi_execute() calls
> scsi_normalize_sense(), which already does
> memset(sshdr, 0, sizeof(struct scsi_sense_hdr)).
__scsi_execute
if (IS_ERR(req)) -->not init
return ret;
if (bufflen && ...) -->not init
goto out;
scsi_normalize_sense
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
2019-10-12 3:25 [PATCH v3] scsi: core: fix uninit-value access of variable sshdr zhengbin
2019-10-17 0:05 ` Finn Thain
@ 2019-10-17 2:45 ` Bart Van Assche
2019-10-17 3:26 ` zhengbin (A)
1 sibling, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2019-10-17 2:45 UTC (permalink / raw)
To: zhengbin, jejb, martin.petersen, linux-scsi; +Cc: yi.zhang
On 2019-10-11 20:25, zhengbin wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 5447738..d5e29c5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> struct scsi_request *rq;
> int ret = DRIVER_ERROR << 24;
>
> + /*
> + * Zero-initialize sshdr for those callers that check the *sshdr
> + * contents even if no sense data is available.
> + */
> + if (sshdr)
> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
> +
> req = blk_get_request(sdev->request_queue,
> data_direction == DMA_TO_DEVICE ?
> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
Although I don't have a strong opinion about this, I'm still wondering
whether 'sshdr' should be initialized in __scsi_execute() or by its caller.
> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
> index ffcf902..335cfdd 100644
> --- a/drivers/scsi/sr_ioctl.c
> +++ b/drivers/scsi/sr_ioctl.c
> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>
> /* Minimal error checking. Ignore cases we know about, and report the rest. */
> if (driver_byte(result) != 0) {
> + if (!scsi_sense_valid(sshdr)) {
> + err = -EIO;
> + goto out;
> + }
> +
> switch (sshdr->sense_key) {
> case UNIT_ATTENTION:
> SDev->changed = 1;
Shouldn't this be a separate patch?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] scsi: core: fix uninit-value access of variable sshdr
2019-10-17 2:45 ` Bart Van Assche
@ 2019-10-17 3:26 ` zhengbin (A)
0 siblings, 0 replies; 5+ messages in thread
From: zhengbin (A) @ 2019-10-17 3:26 UTC (permalink / raw)
To: Bart Van Assche, jejb, martin.petersen, linux-scsi; +Cc: yi.zhang
On 2019/10/17 10:45, Bart Van Assche wrote:
> On 2019-10-11 20:25, zhengbin wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 5447738..d5e29c5 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
>> struct scsi_request *rq;
>> int ret = DRIVER_ERROR << 24;
>>
>> + /*
>> + * Zero-initialize sshdr for those callers that check the *sshdr
>> + * contents even if no sense data is available.
>> + */
>> + if (sshdr)
>> + memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
>> +
>> req = blk_get_request(sdev->request_queue,
>> data_direction == DMA_TO_DEVICE ?
>> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> Although I don't have a strong opinion about this, I'm still wondering
> whether 'sshdr' should be initialized in __scsi_execute() or by its caller.
@jejb, @martin, any suggestion?
>
>> diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
>> index ffcf902..335cfdd 100644
>> --- a/drivers/scsi/sr_ioctl.c
>> +++ b/drivers/scsi/sr_ioctl.c
>> @@ -206,6 +206,11 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
>>
>> /* Minimal error checking. Ignore cases we know about, and report the rest. */
>> if (driver_byte(result) != 0) {
>> + if (!scsi_sense_valid(sshdr)) {
>> + err = -EIO;
>> + goto out;
>> + }
>> +
>> switch (sshdr->sense_key) {
>> case UNIT_ATTENTION:
>> SDev->changed = 1;
> Shouldn't this be a separate patch?
OK, will send a separate patch
>
> Thanks,
>
> Bart.
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-17 3:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-12 3:25 [PATCH v3] scsi: core: fix uninit-value access of variable sshdr zhengbin
2019-10-17 0:05 ` Finn Thain
2019-10-17 1:16 ` zhengbin (A)
2019-10-17 2:45 ` Bart Van Assche
2019-10-17 3:26 ` zhengbin (A)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).