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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 6/28/23 14:36, Kees Cook wrote:
> 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:

I see your point. Yeah; this is one of those cases of nested struct-with-flex-array.

The flex-array is a couple of layers into the main enclosing structure.

However, I would like to have the input of a maintainer here, just to confirm
this would be the expected change.

Thanks a lot for the feedback! :)
--
Gustavo

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