scsi: aacraid: Allocate cmd_priv with scsicmd

PATCH Applied Kees Cook 6 participants 7 messages Jan 28, 2023 – Feb 14, 2023

Subsystems (3)

Stable branch SCSI Aacraid SCSI RAID driver

lore

Share & tools

lore

Kees Cook

author

2023-01-28 00:04 UTC #0 lore raw

PATCH 1/1 scsi: aacraid: Allocate cmd_priv with scsicmd

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13:

../drivers/scsi/aacraid/aachba.c: In function 'aac_probe_container':
../drivers/scsi/aacraid/aachba.c:841:26: warning: array subscript 16 is outside array bounds of 'void[392]' [-Warray-bounds=]
841 | status = cmd_priv->status;
| ^~
In file included from ../include/linux/resource_ext.h:11,

                 from ../include/linux/pci.h:40,
                 from ../drivers/scsi/aacraid/aachba.c:22:

In function 'kmalloc',
inlined from 'kzalloc' at ../include/linux/slab.h:720:9,
inlined from 'aac_probe_container' at ../drivers/scsi/aacraid/aachba.c:821:30:
../include/linux/slab.h:580:24: note: at offset 392 into object of size 392 allocated by 'kmalloc_trace'

  580 |                 return kmalloc_trace(
      |                        ^~~~~~~~~~~~~~
  581 |                                 kmalloc_caches[kmalloc_type(flags)][index],
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  582 |                                 flags, size);
      |                                 ~~~~~~~~~~~~

Fixes 76a3451b64c6 ("scsi: aacraid: Move the SCSI pointer to private command data")

Cc Bart Van Assche <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc Himanshu Madhani <[email protected]>

Cc Adaptec OEM Raid Solutions <[email protected]>

Cc "James E.J. Bottomley" <[email protected]>

Cc "Martin K. Petersen" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/scsi/aacraid/aachba.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/scsi/aacraid/aachba.c +3 -2

index 4d4cb47b3846..24c049eff157 100644
@@ -818,8 +818,8 @@ static void aac_probe_container_scsi_done(struct scsi_cmnd *scsi_cmnd)
 
 int aac_probe_container(struct aac_dev *dev, int cid)
 {
-	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd), GFP_KERNEL);
-	struct aac_cmd_priv *cmd_priv = aac_priv(scsicmd);
+	struct aac_cmd_priv *cmd_priv;
+	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd) + sizeof(*cmd_priv), GFP_KERNEL);
 	struct scsi_device *scsidev = kzalloc(sizeof(*scsidev), GFP_KERNEL);
 	int status;

@@ -838,6 +838,7 @@ int aac_probe_container(struct aac_dev *dev, int cid)
 		while (scsicmd->device == scsidev)
 			schedule();
 	kfree(scsidev);
+	cmd_priv = aac_priv(scsicmd);
 	status = cmd_priv->status;
 	kfree(scsicmd);
 	return status;

Signature · 1 line

2.34.1

Vegard Nossum

Reviewed-by on v1 1/1

2023-01-28 18:40 UTC #1 lore raw

Just a couple of observations.

Quoted 6 lines · Kees Cook

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13:
../drivers/scsi/aacraid/aachba.c: In function 'aac_probe_container':
../drivers/scsi/aacraid/aachba.c:841:26: warning: array subscript 16 is outside array bounds of 'void[392]' [-Warray-bounds=]

An object of size 392 would get allocated with size 512 (at least by SLUB, AFAICT), so the risk of something going terribly wrong is probably fairly small. Not that it shouldn't be fixed, of course...

KASAN should have caught this too, right? Does this mean nobody's tried this driver with KASAN, or is this some kind of rare code path? (Just asking for my own understanding.)

Quoted 10 lines

  int aac_probe_container(struct aac_dev *dev, int cid)
  {

drivers/scsi/aacraid/aachba.c +2 -2

-	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd), GFP_KERNEL);
-	struct aac_cmd_priv *cmd_priv = aac_priv(scsicmd);
+	struct aac_cmd_priv *cmd_priv;
+	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd) + sizeof(*cmd_priv), GFP_KERNEL);
  	struct scsi_device *scsidev = kzalloc(sizeof(*scsidev), GFP_KERNEL);
  	int status;

drivers/scsi/aacraid/aachba.c int aac_probe_container(struct aac_dev *dev, int cid) · line 844

Quoted 7 lines

drivers/scsi/aacraid/aachba.c +1 -0

@@ -838,6 +838,7 @@ int aac_probe_container(struct aac_dev *dev, int cid)
  		while (scsicmd->device == scsidev)
  			schedule();
  	kfree(scsidev);
+	cmd_priv = aac_priv(scsicmd);
  	status = cmd_priv->status;
  	kfree(scsicmd);
  	return status;

aac_priv() uses scsi_cmd_priv() which has the comment:

/*
* Return the driver private allocation behind the command.
* Only works if cmd_size is set in the host template. */

This is set for this driver:

static struct scsi_host_template aac_driver_template = { [...]
.cmd_size = sizeof(struct aac_cmd_priv),

I looked around to see if there was some kind of "allocate cmd" helper, but couldn't find it -- scsi_ioctl_reset() allocates one (together with struct request) and there are a few uses of ->cmd_size in drivers/scsi/scsi_lib.c but there doesn't seem to be a common code path for this.

I guess you could use dev->host->hostt->cmd_size or something, but that doesn't seem worth it since this is driver specific and we already know what the correct value should be.

FWIW:

Reviewed-by Vegard Nossum <[email protected]>

Vegard

John Garry

2023-01-31 09:06 UTC #2 lore raw

Quoted 19 lines · Vegard Nossum

aac_priv() uses scsi_cmd_priv() which has the comment:
/*
* Return the driver private allocation behind the command.
* Only works if cmd_size is set in the host template. */
This is set for this driver:
static struct scsi_host_template aac_driver_template = { [...]
.cmd_size = sizeof(struct aac_cmd_priv),
I looked around to see if there was some kind of "allocate cmd" helper, but couldn't find it -- scsi_ioctl_reset() allocates one (together with struct request) and there are a few uses of ->cmd_size in drivers/scsi/scsi_lib.c but there doesn't seem to be a common code path for this.

Quoted 3 lines

I guess you could use dev->host->hostt->cmd_size or something, but that doesn't seem worth it since this is driver specific and we already know what the correct value should be.

How this driver allocates a SCSI cmd in this fashion is not proper, and hostt->cmd_size would only apply when the SCSI command is allocated in the proper fashion, that being as a request - __scsi_execute() -> scsi_alloc_request() being an example.

Hannes did have a conversion for this driver to allocate a request in

https://urldefense.com/v3/__https://lore.kernel.org/linux-scsi/[email protected]/__;!!ACWV5N9M2RV99hQ!MealB8BN3q8cxYSaB7yKEbHyDmFTNl0YNVQXpVw8Zd0-iNqQ-k4IFxnqONpixfavb0DqGWnkbDVjBJCE22mYq5Ly8Xs$ - hopefully we can progress that work at some stage.

Thanks,
John

Hannes Reinecke

Reviewed-by on v1 1/1

2023-01-30 18:14 UTC #3 lore raw

In reply to Kees Cook

Quoted 52 lines · Kees Cook

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13:
../drivers/scsi/aacraid/aachba.c: In function 'aac_probe_container':
../drivers/scsi/aacraid/aachba.c:841:26: warning: array subscript 16 is outside array bounds of 'void[392]' [-Warray-bounds=]
841 | status = cmd_priv->status;
| ^~
In file included from ../include/linux/resource_ext.h:11,
                  from ../include/linux/pci.h:40,
                  from ../drivers/scsi/aacraid/aachba.c:22:
In function 'kmalloc',
inlined from 'kzalloc' at ../include/linux/slab.h:720:9,
inlined from 'aac_probe_container' at ../drivers/scsi/aacraid/aachba.c:821:30:
../include/linux/slab.h:580:24: note: at offset 392 into object of size 392 allocated by 'kmalloc_trace'
   580 |                 return kmalloc_trace(
       |                        ^~~~~~~~~~~~~~
   581 |                                 kmalloc_caches[kmalloc_type(flags)][index],
       |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   582 |                                 flags, size);
       |                                 ~~~~~~~~~~~~
Fixes 76a3451b64c6 ("scsi: aacraid: Move the SCSI pointer to private command data")
Cc Bart Van Assche <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc Himanshu Madhani <[email protected]>
Cc Adaptec OEM Raid Solutions <[email protected]>
Cc "James E.J. Bottomley" <[email protected]>
Cc "Martin K. Petersen" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
  drivers/scsi/aacraid/aachba.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/scsi/aacraid/aachba.c +2 -2
index 4d4cb47b3846..24c049eff157 100644
@@ -818,8 +818,8 @@ static void aac_probe_container_scsi_done(struct scsi_cmnd *scsi_cmnd)
 
  int aac_probe_container(struct aac_dev *dev, int cid)
  {
-	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd), GFP_KERNEL);
-	struct aac_cmd_priv *cmd_priv = aac_priv(scsicmd);
+	struct aac_cmd_priv *cmd_priv;
+	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd) + sizeof(*cmd_priv), GFP_KERNEL);
  	struct scsi_device *scsidev = kzalloc(sizeof(*scsidev), GFP_KERNEL);
  	int status;
 

drivers/scsi/aacraid/aachba.c int aac_probe_container(struct aac_dev *dev, int cid) · line 844

Quoted 7 lines

drivers/scsi/aacraid/aachba.c +1 -0

@@ -838,6 +838,7 @@ int aac_probe_container(struct aac_dev *dev, int cid)
  		while (scsicmd->device == scsidev)
  			schedule();
  	kfree(scsidev);
+	cmd_priv = aac_priv(scsicmd);
  	status = cmd_priv->status;
  	kfree(scsicmd);
  	return status;

Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 5 lines

Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Bart Van Assche

2023-01-30 18:19 UTC #4 lore raw

In reply to Kees Cook

Quoted 3 lines · Kees Cook

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13: [ ... ]

Bart Van Assche <[email protected]>

Martin K. Petersen

Applied

2023-02-08 23:16 UTC #5 lore raw

In reply to Kees Cook

Kees,

Quoted 3 lines

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13:

Applied to 6.3/scsi-staging, thanks!

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen

Applied

2023-02-14 16:58 UTC #6 lore raw

In reply to Kees Cook

Quoted 15 lines · Kees Cook

The aac_priv() helper assumes that the private cmd area immediately follows struct scsi_cmnd. Allocate this space as part of scsicmd, else there is a risk of heap overflow. Seen with GCC 13:
../drivers/scsi/aacraid/aachba.c: In function 'aac_probe_container':
../drivers/scsi/aacraid/aachba.c:841:26: warning: array subscript 16 is outside array bounds of 'void[392]' [-Warray-bounds=]
841 | status = cmd_priv->status;
| ^~
In file included from ../include/linux/resource_ext.h:11,
                 from ../include/linux/pci.h:40,
                 from ../drivers/scsi/aacraid/aachba.c:22:
In function 'kmalloc',
inlined from 'kzalloc' at ../include/linux/slab.h:720:9,
inlined from 'aac_probe_container' at ../drivers/scsi/aacraid/aachba.c:821:30:
../include/linux/slab.h:580:24: note: at offset 392 into object of size 392 allocated by 'kmalloc_trace'

Quoted 8 lines

  580 |                 return kmalloc_trace(
      |                        ^~~~~~~~~~~~~~
  581 |                                 kmalloc_caches[kmalloc_type(flags)][index],
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  582 |                                 flags, size);
      |                                 ~~~~~~~~~~~~

[...]

Applied to 6.3/scsi-queue, thanks!

[1/1] scsi: aacraid: Allocate cmd_priv with scsicmd

      https://git.kernel.org/mkp/scsi/c/7ab734fc7598

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering