Re: [PATCH][next] scsi: aacraid: Replace one-element array with flexible-array member

[Kees Cook <keescook@chromium.org>]

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