| Message ID | 20230128000409.never.976-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: aacraid: Allocate cmd_priv with scsicmd | expand |
On 1/28/23 01:04, Kees Cook wrote: > 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 <bvanassche@acm.org> > Cc: Hannes Reinecke <hare@suse.de> > Cc: Himanshu Madhani <himanshu.madhani@oracle.com> > Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com> > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com> > Cc: linux-scsi@vger.kernel.org > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/scsi/aacraid/aachba.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c > index 4d4cb47b3846..24c049eff157 100644 > --- a/drivers/scsi/aacraid/aachba.c > +++ b/drivers/scsi/aacraid/aachba.c > @@ -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; Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 28/01/2023 18:40, Vegard Nossum wrote: > 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. 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/8efc0e24-3000-39d9-7676-e0896145f247@suse.de/__;!!ACWV5N9M2RV99hQ!MealB8BN3q8cxYSaB7yKEbHyDmFTNl0YNVQXpVw8Zd0-iNqQ-k4IFxnqONpixfavb0DqGWnkbDVjBJCE22mYq5Ly8Xs$ - hopefully we can progress that work at some stage. Thanks, John
Kees, > 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!
On Fri, 27 Jan 2023 16:04:13 -0800, Kees Cook wrote: > 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); > | ~~~~~~~~~~~~ > > [...] Applied to 6.3/scsi-queue, thanks! [1/1] scsi: aacraid: Allocate cmd_priv with scsicmd https://git.kernel.org/mkp/scsi/c/7ab734fc7598
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c index 4d4cb47b3846..24c049eff157 100644 --- a/drivers/scsi/aacraid/aachba.c +++ b/drivers/scsi/aacraid/aachba.c @@ -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;
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 <bvanassche@acm.org> Cc: Hannes Reinecke <hare@suse.de> Cc: Himanshu Madhani <himanshu.madhani@oracle.com> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com> Cc: "Martin K. Petersen" <martin.petersen@oracle.com> Cc: linux-scsi@vger.kernel.org Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/scsi/aacraid/aachba.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)