Re: [PATCH v1] scsi: libsas: Fix array-bounds warnings

[John Garry <john.garry@huawei.com>]

On 27/04/2022 14:36, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> Use the latest GCC will show below array-bounds warning:

Which version exactly?

> 
> drivers/scsi/libsas/sas_expander.c:1697:39: warning: array subscript ‘struct

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/libsas/sas_expander.c?h=v5.18-rc4#n1679 
is a '}'

What baseline do you use?

> smp_resp[0]’ is partly outside array bounds of ‘unsigned char[56]’ [-Warray-bounds]

I guess that the compiler is getting upset that we're only allocating 32 
bytes for a struct which is 56 bytes in size.

> ...
> drivers/scsi/libsas/sas_expander.c:1781:20: warning: array subscript ‘struct
> smp_resp[0]’ is partly outside array bounds of ‘unsigned char[32]’ [-Warray-bounds]
> ...
> rivers/scsi/libsas/sas_expander.c:1786:39: warning: array subscript ‘struct
> smp_resp[0]’ is partly outside array bounds of ‘unsigned char[32]’ [-Warray-bounds]
> ...
> drivers/scsi/libsas/sas_expander.c:476:35: warning: array subscript ‘struct
> smp_resp[0]’ is partly outside array bounds of ‘unsigned char[32]’ [-Warray-bounds]
> ...
> drivers/scsi/libsas/sas_expander.c:479:38: warning: array subscript ‘struct
> smp_resp[0]’ is partly outside array bounds of ‘unsigned char[32]’ [-Warray-bounds]
> 
> This patch aims to fix these warnings by directly using struct sizes instead of
> macro definitions.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 260e735d06fa..ac6d9be358c5 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -457,7 +457,7 @@ static int sas_ex_general(struct domain_device *dev)
>   	if (!rg_req)
>   		return -ENOMEM;
>   
> -	rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> +	rg_resp = alloc_smp_resp(sizeof(struct smp_resp));

I'm thinking that it's better to have something like:

struct smp_resp_hdr {
	u8    frame_type;
	u8    function;
	u8    result;
	u8    reserved;
};

struct smp_resp {
	union {
		struct report_general_resp  rg;
		struct discover_resp        disc;
		struct report_phy_sata_resp rps;
	};
} __attribute__ ((packed));


struct report_general_resp {
	struct smp_resp_hdr hdr;
	__be16  change_count;
	__be16  route_indexes;
	...
};

or even also get rid of struct smp_resp holder. Sorry if this is more 
than you bargained for, but I don't mind helping.

Thanks,
John

>   	if (!rg_resp) {
>   		kfree(rg_req);
>   		return -ENOMEM;
> @@ -1688,7 +1688,7 @@ static int sas_get_phy_change_count(struct domain_device *dev,
>   	int res;
>   	struct smp_resp *disc_resp;
>   
> -	disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
> +	disc_resp = alloc_smp_resp(sizeof(struct smp_resp));
>   	if (!disc_resp)
>   		return -ENOMEM;
>   
> @@ -1766,7 +1766,7 @@ static int sas_get_ex_change_count(struct domain_device *dev, int *ecc)
>   	if (!rg_req)
>   		return -ENOMEM;
>   
> -	rg_resp = alloc_smp_resp(RG_RESP_SIZE);
> +	rg_resp = alloc_smp_resp(sizeof(struct smp_resp));
>   	if (!rg_resp) {
>   		kfree(rg_req);
>   		return -ENOMEM;