stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vegard Nossum <vegard.nossum@oracle.com>
To: Kees Cook <keescook@chromium.org>, Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>,
	Himanshu Madhani <himanshu.madhani@oracle.com>,
	Adaptec OEM Raid Solutions <aacraid@microsemi.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, stable@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] scsi: aacraid: Allocate cmd_priv with scsicmd
Date: Sat, 28 Jan 2023 19:40:12 +0100	[thread overview]
Message-ID: <1bb8de35-16ba-6c7c-0ef6-67a7226a0a2d@oracle.com> (raw)
In-Reply-To: <20230128000409.never.976-kees@kernel.org>

Just a couple of observations.

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=]

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.)

>   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;

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 <vegard.nossum@oracle.com>


Vegard

  reply	other threads:[~2023-01-28 18:40 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-28  0:04 [PATCH] scsi: aacraid: Allocate cmd_priv with scsicmd Kees Cook
2023-01-28 18:40 ` Vegard Nossum [this message]
2023-01-31  9:02   ` John Garry
2023-01-30 18:14 ` Hannes Reinecke
2023-01-30 18:19 ` Bart Van Assche
2023-02-08 23:16 ` Martin K. Petersen
2023-02-14 16:57 ` Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1bb8de35-16ba-6c7c-0ef6-67a7226a0a2d@oracle.com \
    --to=vegard.nossum@oracle.com \
    --cc=aacraid@microsemi.com \
    --cc=bvanassche@acm.org \
    --cc=hare@suse.de \
    --cc=himanshu.madhani@oracle.com \
    --cc=jejb@linux.ibm.com \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).