| Message ID | 1571387071-28853-1-git-send-email-zhengbin13@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | scsi: core: fix uninit-value access of variable sshdr | expand |
On 10/18/19 10:24 AM, zhengbin wrote: > v1->v2: modify the comments suggested by Bart > v2->v3: fix bug in sr_do_ioctl > v3->v4: let "fix bug in sr_do_ioctl" be a separate patch > v4->v5: fix uninit-value access bug in callers, not in __scsi_execute > > zhengbin (13): > scsi: core: need to check the result of scsi_execute in > scsi_report_opcode > scsi: core: need to check the result of scsi_execute in > scsi_test_unit_ready > scsi: core: need to check the result of scsi_execute in > scsi_report_lun_scan > scsi: sr: need to check the result of scsi_execute in sr_get_events > scsi: sr: need to check the result of scsi_execute in sr_do_ioctl > scsi: scsi_dh_emc: need to check the result of scsi_execute in > send_trespass_cmd > scsi: scsi_dh_rdac: need to check the result of scsi_execute in > send_mode_select > scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in > hp_sw_tur,hp_sw_start_stop > scsi: scsi_dh_alua: need to check the result of scsi_execute in > alua_rtpg,alua_stpg > scsi: scsi_transport_spi: need to check whether sshdr is valid in > spi_execute > scsi: cxlflash: need to check whether sshdr is valid in read_cap16 > scsi: ufs: need to check whether sshdr is valid in > ufshcd_set_dev_pwr_mode > scsi: ch: need to check whether sshdr is valid in ch_do_scsi > > drivers/scsi/ch.c | 6 ++++-- > drivers/scsi/cxlflash/superpipe.c | 2 +- > drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++-- > drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++- > drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++-- > drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++--- > drivers/scsi/scsi.c | 2 +- > drivers/scsi/scsi_lib.c | 3 +++ > drivers/scsi/scsi_scan.c | 3 ++- > drivers/scsi/scsi_transport_spi.c | 1 + > drivers/scsi/sr.c | 3 ++- > drivers/scsi/sr_ioctl.c | 6 ++++++ > drivers/scsi/ufs/ufshcd.c | 3 ++- > 13 files changed, 37 insertions(+), 15 deletions(-) > > -- > 2.7.4 > Nope. The one thing which I patently don't like is the ambivalence between DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ of them is set? IE what would be the correct way of action if DRIVER_SENSE is not set, but we have a valid sense code? Or the other way around? But more important, from a quick glance not all drivers set the DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is never evaluated after this patchset. I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on scsi_sense_valid() only. Cheers, Hannes
On 2019/10/18 17:17, zhengbin wrote: > v1->v2: modify the comments suggested by Bart > v2->v3: fix bug in sr_do_ioctl > v3->v4: let "fix bug in sr_do_ioctl" be a separate patch > v4->v5: fix uninit-value access bug in callers, not in __scsi_execute An explanation text of the series would be great... What about defining a little helper function for checking driver_byte(err) == DRIVER_SENSE && scsi_sense_valid(&sshdr) instead of having that hard-coded everywhere ? That would make the code a lot cleaner and more readable. Something like: static inline bool scsi_driver_sense_valid(int result, struct scsi_sense_hdr *sshdr) { return driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(sshdr); } > > zhengbin (13): > scsi: core: need to check the result of scsi_execute in > scsi_report_opcode > scsi: core: need to check the result of scsi_execute in > scsi_test_unit_ready > scsi: core: need to check the result of scsi_execute in > scsi_report_lun_scan > scsi: sr: need to check the result of scsi_execute in sr_get_events > scsi: sr: need to check the result of scsi_execute in sr_do_ioctl > scsi: scsi_dh_emc: need to check the result of scsi_execute in > send_trespass_cmd > scsi: scsi_dh_rdac: need to check the result of scsi_execute in > send_mode_select > scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in > hp_sw_tur,hp_sw_start_stop > scsi: scsi_dh_alua: need to check the result of scsi_execute in > alua_rtpg,alua_stpg > scsi: scsi_transport_spi: need to check whether sshdr is valid in > spi_execute > scsi: cxlflash: need to check whether sshdr is valid in read_cap16 > scsi: ufs: need to check whether sshdr is valid in > ufshcd_set_dev_pwr_mode > scsi: ch: need to check whether sshdr is valid in ch_do_scsi > > drivers/scsi/ch.c | 6 ++++-- > drivers/scsi/cxlflash/superpipe.c | 2 +- > drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++-- > drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++- > drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++-- > drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++--- > drivers/scsi/scsi.c | 2 +- > drivers/scsi/scsi_lib.c | 3 +++ > drivers/scsi/scsi_scan.c | 3 ++- > drivers/scsi/scsi_transport_spi.c | 1 + > drivers/scsi/sr.c | 3 ++- > drivers/scsi/sr_ioctl.c | 6 ++++++ > drivers/scsi/ufs/ufshcd.c | 3 ++- > 13 files changed, 37 insertions(+), 15 deletions(-) > > -- > 2.7.4 > >
Hannes, > The one thing which I patently don't like is the ambivalence between > DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ > of them is set? IE what would be the correct way of action if > DRIVER_SENSE is not set, but we have a valid sense code? Or the other > way around? I agree, it's a mess. (Sorry, zhengbin, you opened a can of worms. This is some of our oldest and most arcane code in SCSI) > But more important, from a quick glance not all drivers set the > DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is > never evaluated after this patchset. And yet we appear to have several code paths where sense evaluation is contingent on DRIVER_SENSE. So no matter what, behavior might change if we enforce consistent semantics. *sigh* > I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on > scsi_sense_valid() only. I would really like to get rid of DRIVER_* completely. Except for DRIVER_SENSE, few are actually in use: DRIVER_OK: 0 DRIVER_BUSY: 0 DRIVER_SOFT: 0 DRIVER_MEDIA: 0 DRIVER_ERROR: 6 DRIVER_INVALID: 4 DRIVER_TIMEOUT: 1 DRIVER_SENSE: 58 Johannes: Whatever happened to your efforts at cleaning all this up? Do you have a patch series or a working tree we could use as starting point?
On 10/18/19 3:43 PM, Martin K. Petersen wrote: > > Hannes, > >> The one thing which I patently don't like is the ambivalence between >> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >> of them is set? IE what would be the correct way of action if >> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >> way around? > > I agree, it's a mess. > > (Sorry, zhengbin, you opened a can of worms. This is some of our oldest > and most arcane code in SCSI) > >> But more important, from a quick glance not all drivers set the >> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >> never evaluated after this patchset. > > And yet we appear to have several code paths where sense evaluation is > contingent on DRIVER_SENSE. So no matter what, behavior might > change if we enforce consistent semantics. *sigh* > >> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on >> scsi_sense_valid() only. > > I would really like to get rid of DRIVER_* completely. Except for > DRIVER_SENSE, few are actually in use: > > DRIVER_OK: 0 > DRIVER_BUSY: 0 > DRIVER_SOFT: 0 > DRIVER_MEDIA: 0 > DRIVER_ERROR: 6 Three less now :-) Cheers, Hannes
On 2019/10/18 21:43, Martin K. Petersen wrote: > Hannes, > >> The one thing which I patently don't like is the ambivalence between >> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >> of them is set? IE what would be the correct way of action if >> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >> way around? > I agree, it's a mess. > > (Sorry, zhengbin, you opened a can of worms. This is some of our oldest > and most arcane code in SCSI) > >> But more important, from a quick glance not all drivers set the >> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >> never evaluated after this patchset. > And yet we appear to have several code paths where sense evaluation is > contingent on DRIVER_SENSE. So no matter what, behavior might > change if we enforce consistent semantics. *sigh* So what should we do to prevent unit-value access of sshdr? 1. still init sshdr in __scsi_execute? @@ -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); 2. init sshdr in callers of scsi_execute, instead of check the result of scsi_execute and check whether sshdr is valid? for example: @@ -506,6 +506,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer, put_unaligned_be32(len, &cmd[6]); memset(buffer, 0, len); + memset(&sshdr, 0 ,sizeof(sshdr)); result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len, &sshdr, 30 * HZ, 3, NULL) Besides, should sd_pr_command init sshdr?? cause sd_pr_command have checked the result of scsi_execute and check whether sshdr is valid. 3. get rid of DRIVER_* completely? even this, we should init sshdr first. sshdr is just 8 bytes, init it does not affect performance My advice is 2. > >> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on >> scsi_sense_valid() only. > I would really like to get rid of DRIVER_* completely. Except for > DRIVER_SENSE, few are actually in use: > > DRIVER_OK: 0 > DRIVER_BUSY: 0 > DRIVER_SOFT: 0 > DRIVER_MEDIA: 0 > DRIVER_ERROR: 6 > DRIVER_INVALID: 4 > DRIVER_TIMEOUT: 1 > DRIVER_SENSE: 58 > > Johannes: Whatever happened to your efforts at cleaning all this up? Do > you have a patch series or a working tree we could use as starting > point? >
On 18/10/2019 15:43, Martin K. Petersen wrote: [...] > Johannes: Whatever happened to your efforts at cleaning all this up? Do > you have a patch series or a working tree we could use as starting > point? I'll have to look. I think it's still floating around in some git tree. Let me search for it. Johannes
On 10/21/19 3:49 AM, zhengbin (A) wrote: > > On 2019/10/18 21:43, Martin K. Petersen wrote: >> Hannes, >> >>> The one thing which I patently don't like is the ambivalence between >>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >>> of them is set? IE what would be the correct way of action if >>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >>> way around? >> I agree, it's a mess. >> >> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >> and most arcane code in SCSI) >> >>> But more important, from a quick glance not all drivers set the >>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >>> never evaluated after this patchset. >> And yet we appear to have several code paths where sense evaluation is >> contingent on DRIVER_SENSE. So no matter what, behavior might >> change if we enforce consistent semantics. *sigh* > > So what should we do to prevent unit-value access of sshdr? > Where do you see it? From my reading, __scsi_execute() is clearing sshdr by way of __scsi_execute() -> scsi_normalize_sense() -> memset(sshdr) Cheers, Hannes
On 2019/10/21 21:06, Hannes Reinecke wrote: > On 10/21/19 3:49 AM, zhengbin (A) wrote: >> On 2019/10/18 21:43, Martin K. Petersen wrote: >>> Hannes, >>> >>>> The one thing which I patently don't like is the ambivalence between >>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >>>> of them is set? IE what would be the correct way of action if >>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >>>> way around? >>> I agree, it's a mess. >>> >>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >>> and most arcane code in SCSI) >>> >>>> But more important, from a quick glance not all drivers set the >>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >>>> never evaluated after this patchset. >>> And yet we appear to have several code paths where sense evaluation is >>> contingent on DRIVER_SENSE. So no matter what, behavior might >>> change if we enforce consistent semantics. *sigh* >> So what should we do to prevent unit-value access of sshdr? >> > Where do you see it? > >From my reading, __scsi_execute() is clearing sshdr by way of > > __scsi_execute() > -> scsi_normalize_sense() > -> memset(sshdr) __scsi_execute req = blk_get_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); if (IS_ERR(req)) return ret; -->just return rq = scsi_req(req); if (bufflen && blk_rq_map_kern(sdev->request_queue, req, buffer, bufflen, GFP_NOIO)) goto out; -->just goto out > > Cheers, > > Hannes
On 2019/10/22 9:59, zhengbin (A) wrote: > On 2019/10/21 21:06, Hannes Reinecke wrote: >> On 10/21/19 3:49 AM, zhengbin (A) wrote: >>> On 2019/10/18 21:43, Martin K. Petersen wrote: >>>> Hannes, >>>> >>>>> The one thing which I patently don't like is the ambivalence between >>>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_ >>>>> of them is set? IE what would be the correct way of action if >>>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other >>>>> way around? >>>> I agree, it's a mess. >>>> >>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest >>>> and most arcane code in SCSI) >>>> >>>>> But more important, from a quick glance not all drivers set the >>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is >>>>> never evaluated after this patchset. >>>> And yet we appear to have several code paths where sense evaluation is >>>> contingent on DRIVER_SENSE. So no matter what, behavior might >>>> change if we enforce consistent semantics. *sigh* >>> So what should we do to prevent unit-value access of sshdr? >>> >> Where do you see it? >> >From my reading, __scsi_execute() is clearing sshdr by way of >> >> __scsi_execute() >> -> scsi_normalize_sense() >> -> memset(sshdr) > __scsi_execute > > req = blk_get_request(sdev->request_queue, > data_direction == DMA_TO_DEVICE ? > REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); > if (IS_ERR(req)) > return ret; -->just return > rq = scsi_req(req); > > if (bufflen && blk_rq_map_kern(sdev->request_queue, req, > buffer, bufflen, GFP_NOIO)) > goto out; -->just goto out may be we should init sshdr in __scsi_execute? which is the simplest way, and do not lose anyone. If we init sshdr in the callers, maybe we will lose some function. + /* + * 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)); + > >> Cheers, >> >> Hannes
v1->v2: modify the comments suggested by Bart v2->v3: fix bug in sr_do_ioctl v3->v4: let "fix bug in sr_do_ioctl" be a separate patch v4->v5: fix uninit-value access bug in callers, not in __scsi_execute zhengbin (13): scsi: core: need to check the result of scsi_execute in scsi_report_opcode scsi: core: need to check the result of scsi_execute in scsi_test_unit_ready scsi: core: need to check the result of scsi_execute in scsi_report_lun_scan scsi: sr: need to check the result of scsi_execute in sr_get_events scsi: sr: need to check the result of scsi_execute in sr_do_ioctl scsi: scsi_dh_emc: need to check the result of scsi_execute in send_trespass_cmd scsi: scsi_dh_rdac: need to check the result of scsi_execute in send_mode_select scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop scsi: scsi_dh_alua: need to check the result of scsi_execute in alua_rtpg,alua_stpg scsi: scsi_transport_spi: need to check whether sshdr is valid in spi_execute scsi: cxlflash: need to check whether sshdr is valid in read_cap16 scsi: ufs: need to check whether sshdr is valid in ufshcd_set_dev_pwr_mode scsi: ch: need to check whether sshdr is valid in ch_do_scsi drivers/scsi/ch.c | 6 ++++-- drivers/scsi/cxlflash/superpipe.c | 2 +- drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++-- drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++- drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++-- drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++--- drivers/scsi/scsi.c | 2 +- drivers/scsi/scsi_lib.c | 3 +++ drivers/scsi/scsi_scan.c | 3 ++- drivers/scsi/scsi_transport_spi.c | 1 + drivers/scsi/sr.c | 3 ++- drivers/scsi/sr_ioctl.c | 6 ++++++ drivers/scsi/ufs/ufshcd.c | 3 ++- 13 files changed, 37 insertions(+), 15 deletions(-) -- 2.7.4