Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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 -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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 -- Johannes ThumshirnSUSE Labs Filesystems jthumsh...@suse.de+49 911 74053 689 SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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? >
[RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
On Fri, 18 Oct 2019, Martin K. Petersen wrote:
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is
set clobbers the most significant bytes in cmd->result.
Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.
This is a theoretical bug, found by inspection, so I'm sending an untested
RFC patch.
I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@ static inline struct scsi_data_buffer *scsi_prot(struct
scsi_cmnd *cmd)
#define scsi_for_each_prot_sg(cmd, sg, nseg, __i) \
for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0x00ff) | (status << 8);
}
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0xff00) | (status << 16);
}
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0x00ff) | (status << 24);
}
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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 -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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? -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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
>
>
--
Damien Le Moal
Western Digital Research
Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
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 -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 247165 (AG München), GF: Felix Imendörffer
