Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 5:05 AM, zhengbin wrote: + /* +* need to initial sshdr to avoid uninit-value access +*/ + if (sshdr) + memset(sshdr, 0, sizeof(struct scsi_sense_hdr)); + I think the above comment is slightly confusing because it is correct for some callers but not for all callers of scsi_execute(). How about changing the comment into something like the following: "Zero-initialize sshdr for those callers that check the *sshdr contents even if no sense data is available"? Thanks, Bart.
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 8:07 PM, zhengbin (A) wrote: Besides, scsi_sense_hdr is just 8 bytes, memset it to 0 will not affect performance That's true ... Bart.
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 2019/10/11 1:03, Bart Van Assche wrote: > On 10/10/19 5:05 AM, 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: we can't just init sshdr->response_code, sr_do_ioctl use >> sshdr->sense_key(Need to troubleshoot all callers) >> >> Signed-off-by: zhengbin >> --- >> drivers/scsi/scsi_lib.c | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 5447738..037fb2a 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -255,6 +255,12 @@ int __scsi_execute(struct scsi_device *sdev, const >> unsigned char *cmd, >> struct scsi_request *rq; >> int ret = DRIVER_ERROR << 24; >> >> + /* >> + * need to initial sshdr to avoid uninit-value access >> + */ >> + 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); > > From SAM-5: "Sense data shall be made available by the logical unit if a > command terminates with CHECK CONDITION status or other conditions (e.g., the > processing of a REQUEST SENSE command). The format, content, and conditions > under which sense data shall be prepared by the logical unit are as defined > in this standard, SPC-4, the applicable command standard, and the applicable > SCSI transport protocol standard." Even this, I suggest we should still init sshdr first(this is hard to understand) > > Apparently sr_check_events() submits a GET EVENT STATUS NOTIFICATION command > and it even evaluates the sense data if that command succeeded. I'm not aware > of other scsi_execute() callers that do this. So I'm not sure that modifying > __scsi_execute() is the best approach. I'm wondering whether the sr driver > should be fixed instead of modifying __scsi_execute(). I have troubleshoot callers, there are similar bug(scsi_report_opcode, cache_type_store, scsi_test_unit_ready, scsi_report_lun_scan, sd_spinup_disk, read_capacity_16, read_capacity_10, sr_get_events, alua_rtpg, alua_stpg, send_trespass_cmd, hp_sw_tur, hp_sw_start_stop, send_mode_select, sd_sync_cache, sd_start_stop_device, sr_do_ioctl). In __scsi_execute, if a command was executed, sshdr was set, so if failed, init sshdr should be ok too. Besides, scsi_sense_hdr is just 8 bytes, memset it to 0 will not affect performance > > Thanks, > > Bart. > > . >
Re: [PATCH] scsi: core: fix uninit-value access of variable sshdr
On 10/10/19 5:05 AM, 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: GB 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: we can't just init sshdr->response_code, sr_do_ioctl use sshdr->sense_key(Need to troubleshoot all callers) Signed-off-by: zhengbin --- drivers/scsi/scsi_lib.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 5447738..037fb2a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -255,6 +255,12 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, struct scsi_request *rq; int ret = DRIVER_ERROR << 24; + /* +* need to initial sshdr to avoid uninit-value access +*/ + 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); From SAM-5: "Sense data shall be made available by the logical unit if a command terminates with CHECK CONDITION status or other conditions (e.g., the processing of a REQUEST SENSE command). The format, content, and conditions under which sense data shall be prepared by the logical unit are as defined in this standard, SPC-4, the applicable command standard, and the applicable SCSI transport protocol standard." Apparently sr_check_events() submits a GET EVENT STATUS NOTIFICATION command and it even evaluates the sense data if that command succeeded. I'm not aware of other scsi_execute() callers that do this. So I'm not sure that modifying __scsi_execute() is the best approach. I'm wondering whether the sr driver should be fixed instead of modifying __scsi_execute(). Thanks, Bart.
[PATCH] scsi: core: fix uninit-value access of variable sshdr
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: GB 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: we can't just init sshdr->response_code, sr_do_ioctl use sshdr->sense_key(Need to troubleshoot all callers) Signed-off-by: zhengbin --- drivers/scsi/scsi_lib.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 5447738..037fb2a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -255,6 +255,12 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, struct scsi_request *rq; int ret = DRIVER_ERROR << 24; + /* +* need to initial sshdr to avoid uninit-value access +*/ + 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); -- 2.7.4
