[v5,00/13] scsi: core: fix uninit-value access of variable sshdr
mbox series

Message ID 1571387071-28853-1-git-send-email-zhengbin13@huawei.com
Headers show
Series
  • scsi: core: fix uninit-value access of variable sshdr
Related show

Message

zhengbin (A) Oct. 18, 2019, 8:24 a.m. UTC
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

Comments

Hannes Reinecke Oct. 18, 2019, 9:41 a.m. UTC | #1
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
Damien Le Moal Oct. 18, 2019, 12:33 p.m. UTC | #2
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
> 
>
Martin K. Petersen Oct. 18, 2019, 1:43 p.m. UTC | #3
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?
Hannes Reinecke Oct. 18, 2019, 2:05 p.m. UTC | #4
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
zhengbin (A) Oct. 21, 2019, 1:49 a.m. UTC | #5
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?
>
Johannes Thumshirn Oct. 21, 2019, 6:57 a.m. UTC | #6
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
Hannes Reinecke Oct. 21, 2019, 1:06 p.m. UTC | #7
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
zhengbin (A) Oct. 22, 2019, 1:59 a.m. UTC | #8
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
zhengbin (A) Oct. 23, 2019, 6:51 a.m. UTC | #9
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