linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: 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, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member
Date: Wed, 7 Apr 2021 12:22:52 -0700	[thread overview]
Message-ID: <202104071216.5BEA350@keescook> (raw)
In-Reply-To: <20210304203822.GA102218@embeddedor>

On Thu, Mar 04, 2021 at 02:38:22PM -0600, Gustavo A. R. Silva wrote:
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code according to the use of a flexible-array member in
> struct aac_raw_io2 instead of one-element array, and use the
> struct_size() and flex_array_size() helpers.
> 
> Also, this helps with the ongoing efforts to enable -Warray-bounds by
> fixing the following warnings:
> 
> drivers/scsi/aacraid/aachba.c: In function ‘aac_build_sgraw2’:
> drivers/scsi/aacraid/aachba.c:3970:18: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  3970 |     if (rio2->sge[j].length % (i*PAGE_SIZE)) {
>       |         ~~~~~~~~~^~~
> drivers/scsi/aacraid/aachba.c:3974:27: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  3974 |     nseg_new += (rio2->sge[j].length / (i*PAGE_SIZE));
>       |                  ~~~~~~~~~^~~
> drivers/scsi/aacraid/aachba.c:4011:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  4011 |   for (j = 0; j < rio2->sge[i].length / (pages * PAGE_SIZE); ++j) {
>       |                   ~~~~~~~~~^~~
> drivers/scsi/aacraid/aachba.c:4012:24: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  4012 |    addr_low = rio2->sge[i].addrLow + j * pages * PAGE_SIZE;
>       |               ~~~~~~~~~^~~
> drivers/scsi/aacraid/aachba.c:4014:33: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  4014 |    sge[pos].addrHigh = rio2->sge[i].addrHigh;
>       |                        ~~~~~~~~~^~~
> drivers/scsi/aacraid/aachba.c:4015:28: warning: array subscript 1 is above array bounds of ‘struct sge_ieee1212[1]’ [-Warray-bounds]
>  4015 |    if (addr_low < rio2->sge[i].addrLow)
>       |                   ~~~~~~~~~^~~
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Build-tested-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/lkml/60414244.ur4%2FkI+fBF1ohKZs%25lkp@intel.com/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/scsi/aacraid/aachba.c  | 13 +++++++------
>  drivers/scsi/aacraid/aacraid.h |  2 +-
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 4ca5e13a26a6..0f5617e40b94 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1235,8 +1235,8 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>  		if (ret < 0)
>  			return ret;
>  		command = ContainerRawIo2;
> -		fibsize = sizeof(struct aac_raw_io2) +
> -			((le32_to_cpu(readcmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212));
> +		fibsize = struct_size(readcmd2, sge,
> +				     le32_to_cpu(readcmd2->sgeCnt));

readcmd2 is struct aac_raw_io2, and sge is the struct sge_ieee1212
array, so this looks correct to me with the change to struct
aac_raw_io2..

>  	} else {
>  		struct aac_raw_io *readcmd;
>  		readcmd = (struct aac_raw_io *) fib_data(fib);
> @@ -1366,8 +1366,8 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
>  		if (ret < 0)
>  			return ret;
>  		command = ContainerRawIo2;
> -		fibsize = sizeof(struct aac_raw_io2) +
> -			((le32_to_cpu(writecmd2->sgeCnt)-1) * sizeof(struct sge_ieee1212));
> +		fibsize = struct_size(writecmd2, sge,
> +				      le32_to_cpu(writecmd2->sgeCnt));

writecmd2 is struct aac_raw_io2, and sge is the struct sge_ieee1212
array, so this looks correct to me with the change to struct
aac_raw_io2.

>  	} else {
>  		struct aac_raw_io *writecmd;
>  		writecmd = (struct aac_raw_io *) fib_data(fib);
> @@ -4003,7 +4003,7 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int
>  	if (aac_convert_sgl == 0)
>  		return 0;
>  
> -	sge = kmalloc_array(nseg_new, sizeof(struct sge_ieee1212), GFP_ATOMIC);
> +	sge = kmalloc_array(nseg_new, sizeof(*sge), GFP_ATOMIC);

Technically, this is unrelated (struct sge_ieee1212 has not changed),
but sge is a struct sge_ieee1212 pointer, so this is good robustness
change, IMO.

>  	if (sge == NULL)
>  		return -ENOMEM;
>  
> @@ -4020,7 +4020,8 @@ static int aac_convert_sgraw2(struct aac_raw_io2 *rio2, int pages, int nseg, int
>  		}
>  	}
>  	sge[pos] = rio2->sge[nseg-1];
> -	memcpy(&rio2->sge[1], &sge[1], (nseg_new-1)*sizeof(struct sge_ieee1212));
> +	memcpy(&rio2->sge[1], &sge[1],
> +	       flex_array_size(rio2, sge, nseg_new - 1));

This was hard to validate, but looks correct to me. The flex array
helper here is the same as the prior code (but now tied to the
variables, which is more robust IMO). The use of seg[1] here appears to
be just how this code works -- the loop above is rewriting the 1 through
nseg_new - 1 array entries, and then this copies back the results.

>  
>  	kfree(sge);
>  	rio2->sgeCnt = cpu_to_le32(nseg_new);
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index e3e4ecbea726..3733df77bc65 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -1929,7 +1929,7 @@ struct aac_raw_io2 {
>  	u8		bpComplete;	/* reserved for F/W use */
>  	u8		sgeFirstIndex;	/* reserved for F/W use */
>  	u8		unused[4];
> -	struct sge_ieee1212	sge[1];
> +	struct sge_ieee1212	sge[];
>  };
>  
>  #define CT_FLUSH_CACHE 129
> -- 
> 2.27.0
> 

Thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

  parent reply	other threads:[~2021-04-07 19:22 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-04 20:38 Gustavo A. R. Silva
2021-03-25  1:18 ` Martin K. Petersen
2021-03-25  0:46   ` Gustavo A. R. Silva
2021-03-26  3:34     ` Martin K. Petersen
2021-03-26  3:07       ` Gustavo A. R. Silva
2021-04-07 19:22 ` Kees Cook [this message]
2021-04-13  4:52   ` Martin K. Petersen
2021-04-13  5:45     ` Gustavo A. R. Silva
2021-04-13 14:04       ` James Bottomley

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=202104071216.5BEA350@keescook \
    --to=keescook@chromium.org \
    --cc=aacraid@microsemi.com \
    --cc=gustavoars@kernel.org \
    --cc=jejb@linux.ibm.com \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --subject='Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member' \
    /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

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