| Message ID | 0c7402fe6448186cda5a2618a35eb5f8d1cbb313.1687974498.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: aacraid: Replace one-element arrays with flexible-array members | expand |
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 >
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 >> >
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;
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. 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(-)