Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap

[Kees Cook <keescook@chromium.org>]

On Wed, Jun 28, 2023 at 11:57:13AM -0600, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> sgmap and refactor the rest of the code, accordingly.
> 
> Issue found with the help of Coccinelle and audited and fixed,
> manually.

This change _does_ have binary output differences, although it looks
like you got most of them. I still see:

commsup.o:
-       mov    $0x40,%edx
+       mov    $0x38,%edx

This appears to be the sizeof() here:

        ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
                                FsaNormal, 1, 1, NULL, NULL);

struct aac_srb includes struct sgmap. I think this needs to explicitly
include the 1 sgmap, which seems to be sent in the fibptr:

        srbcmd = (struct aac_srb *)fib_data(fibptr);
	...
        sg64 = (struct sgmap64 *)&srbcmd->sg;
        sg64->count = cpu_to_le32(1);

i.e. "sending 1". This seems to fix it:

-       ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
+       ret = aac_fib_send(ScsiPortCommand64, fibptr,
+                               struct_size(srbcmd, sg.sg, 1),

Then I see changes in both aac_write_block() and aac_scsi_32(), but they
match the changes you made to get the correct size (it's just an easier
calculation for the compiler to perform, so the code is slightly
simplified).

So I think with the hunk I suggested at the start, and a comment on the
(expected) binary changes, this should be good to go.

-Kees

> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/ClangBuiltLinux/linux/issues/1851
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/scsi/aacraid/aachba.c   | 24 ++++++++++--------------
>  drivers/scsi/aacraid/aacraid.h  |  2 +-
>  drivers/scsi/aacraid/commctrl.c |  4 ++--
>  drivers/scsi/aacraid/comminit.c |  3 +--
>  4 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 03ba974f6b2a..b2849e5cc104 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -1336,8 +1336,7 @@ static int aac_read_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u32
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_read) +
> -			((le32_to_cpu(readcmd->sg.count) - 1) *
> -			 sizeof (struct sgentry));
> +		  le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -1471,8 +1470,7 @@ static int aac_write_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
>  	if (ret < 0)
>  		return ret;
>  	fibsize = sizeof(struct aac_write) +
> -		((le32_to_cpu(writecmd->sg.count) - 1) *
> -		 sizeof (struct sgentry));
> +		  le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  	/*
> @@ -1590,9 +1588,9 @@ static int aac_scsi_64(struct fib * fib, struct scsi_cmnd * cmd)
>  	/*
>  	 *	Build Scatter/Gather list
>  	 */
> -	fibsize = sizeof (struct aac_srb) - sizeof (struct sgentry) +
> -		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
> -		 sizeof (struct sgentry64));
> +	fibsize = sizeof(struct aac_srb) +
> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
> +		  sizeof(struct sgentry64);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  
> @@ -1621,9 +1619,9 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
>  	/*
>  	 *	Build Scatter/Gather list
>  	 */
> -	fibsize = sizeof (struct aac_srb) +
> -		(((le32_to_cpu(srbcmd->sg.count) & 0xff) - 1) *
> -		 sizeof (struct sgentry));
> +	fibsize = sizeof(struct aac_srb) +
> +		  (le32_to_cpu(srbcmd->sg.count) & 0xff) *
> +		  sizeof(struct sgentry);
>  	BUG_ON (fibsize > (fib->dev->max_fib_size -
>  				sizeof(struct aac_fibhdr)));
>  
> @@ -1691,8 +1689,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
>  	fibptr->hw_fib_va->header.XferState &=
>  		~cpu_to_le32(FastResponseCapable);
>  
> -	fibsize  = sizeof(struct aac_srb) - sizeof(struct sgentry) +
> -						sizeof(struct sgentry64);
> +	fibsize  = sizeof(struct aac_srb) + sizeof(struct sgentry64);
>  
>  	/* allocate DMA buffer for response */
>  	addr = dma_map_single(&dev->pdev->dev, xfer_buf, xfer_len,
> @@ -2264,8 +2261,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
>  		dev->a_ops.adapter_bounds = aac_bounds_32;
>  		dev->scsi_host_ptr->sg_tablesize = (dev->max_fib_size -
>  			sizeof(struct aac_fibhdr) -
> -			sizeof(struct aac_write) + sizeof(struct sgentry)) /
> -				sizeof(struct sgentry);
> +			sizeof(struct aac_write)) / sizeof(struct sgentry);
>  		if (dev->dac_support) {
>  			dev->a_ops.adapter_read = aac_read_block64;
>  			dev->a_ops.adapter_write = aac_write_block64;
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 94eb83d38be6..3fbc22ae72b6 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -507,7 +507,7 @@ struct sge_ieee1212 {
>  
>  struct sgmap {
>  	__le32		count;
> -	struct sgentry	sg[1];
> +	struct sgentry	sg[];
>  };
>  
>  struct user_sgmap {
> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index e7cc927ed952..df811ad4afaa 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -561,8 +561,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  		rcode = -EINVAL;
>  		goto cleanup;
>  	}
> -	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
> -		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
> +	actual_fibsize = sizeof(struct aac_srb) +
> +		(user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry);
>  	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
>  	  (sizeof(struct sgentry64) - sizeof(struct sgentry));
>  	/* User made a mistake - should not continue */
> diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
> index bd99c5492b7d..d8dd89c87b01 100644
> --- a/drivers/scsi/aacraid/comminit.c
> +++ b/drivers/scsi/aacraid/comminit.c
> @@ -523,8 +523,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>  	dev->max_fib_size = sizeof(struct hw_fib);
>  	dev->sg_tablesize = host->sg_tablesize = (dev->max_fib_size
>  		- sizeof(struct aac_fibhdr)
> -		- sizeof(struct aac_write) + sizeof(struct sgentry))
> -			/ sizeof(struct sgentry);
> +		- sizeof(struct aac_write)) / sizeof(struct sgentry);
>  	dev->comm_interface = AAC_COMM_PRODUCER;
>  	dev->raw_io_interface = dev->raw_io_64 = 0;
>  
> -- 
> 2.34.1
> 

-- 
Kees Cook