Re: [PATCH 08/10][next] scsi: aacraid: Replace one-element array with flexible-array member in struct sgmap
From: Kees Cook
Date: Wed Jun 28 2023 - 16:39:54 EST
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@xxxxxxxxxx>
> ---
> 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