| Message ID | 20220112220652.3952944-1-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] sata_fsl: Use struct_group() for memcpy() region | expand |
On 1/12/22 3:06 PM, Kees Cook wrote: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. > > Use struct_group() in struct command_desc around members acmd and fill, > so they can be referenced together. This will allow memset(), memcpy(), > and sizeof() to more easily reason about sizes, improve readability, > and avoid future warnings about writing beyond the end of acmd: > > In function 'fortify_memset_chk', > inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: > ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] > 199 | __write_overflow_field(); > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > Cc: Jens Axboe <axboe@kernel.dk> > Cc: linux-ide@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE > improvements that are close to being finished. :) I don't maintain libata anymore, so Damien is the guy to nudge ;-)
On Wed, Jan 12, 2022 at 03:23:40PM -0700, Jens Axboe wrote: > On 1/12/22 3:06 PM, Kees Cook wrote: > > In preparation for FORTIFY_SOURCE performing compile-time and run-time > > field bounds checking for memcpy(), memmove(), and memset(), avoid > > intentionally writing across neighboring fields. > > > > Use struct_group() in struct command_desc around members acmd and fill, > > so they can be referenced together. This will allow memset(), memcpy(), > > and sizeof() to more easily reason about sizes, improve readability, > > and avoid future warnings about writing beyond the end of acmd: > > > > In function 'fortify_memset_chk', > > inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: > > ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] > > 199 | __write_overflow_field(); > > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > > > Cc: Jens Axboe <axboe@kernel.dk> > > Cc: linux-ide@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE > > improvements that are close to being finished. :) > > I don't maintain libata anymore, so Damien is the guy to nudge ;-) Ah-ha, okay, thanks. /me waves "hi" to Damien. :)
On 1/13/22 08:15, Kees Cook wrote: > On Wed, Jan 12, 2022 at 03:23:40PM -0700, Jens Axboe wrote: >> On 1/12/22 3:06 PM, Kees Cook wrote: >>> In preparation for FORTIFY_SOURCE performing compile-time and run-time >>> field bounds checking for memcpy(), memmove(), and memset(), avoid >>> intentionally writing across neighboring fields. >>> >>> Use struct_group() in struct command_desc around members acmd and fill, >>> so they can be referenced together. This will allow memset(), memcpy(), >>> and sizeof() to more easily reason about sizes, improve readability, >>> and avoid future warnings about writing beyond the end of acmd: >>> >>> In function 'fortify_memset_chk', >>> inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: >>> ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] >>> 199 | __write_overflow_field(); >>> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Cc: Jens Axboe <axboe@kernel.dk> >>> Cc: linux-ide@vger.kernel.org >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE >>> improvements that are close to being finished. :) >> >> I don't maintain libata anymore, so Damien is the guy to nudge ;-) > > Ah-ha, okay, thanks. > > /me waves "hi" to Damien. :) Hi Kees, This is already queued up in libata tree for-5.17 branch. I have not sent my PR to Linus yet as I am letting things soack a little longer in for-next (for the various arch compile tests). Please check that branch to see if all is OK ! Cheers.
On Thu, Jan 13, 2022 at 08:47:37AM +0900, Damien Le Moal wrote: > On 1/13/22 08:15, Kees Cook wrote: > > On Wed, Jan 12, 2022 at 03:23:40PM -0700, Jens Axboe wrote: > >> On 1/12/22 3:06 PM, Kees Cook wrote: > >>> In preparation for FORTIFY_SOURCE performing compile-time and run-time > >>> field bounds checking for memcpy(), memmove(), and memset(), avoid > >>> intentionally writing across neighboring fields. > >>> > >>> Use struct_group() in struct command_desc around members acmd and fill, > >>> so they can be referenced together. This will allow memset(), memcpy(), > >>> and sizeof() to more easily reason about sizes, improve readability, > >>> and avoid future warnings about writing beyond the end of acmd: > >>> > >>> In function 'fortify_memset_chk', > >>> inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: > >>> ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] > >>> 199 | __write_overflow_field(); > >>> | ^~~~~~~~~~~~~~~~~~~~~~~~ > >>> > >>> Cc: Jens Axboe <axboe@kernel.dk> > >>> Cc: linux-ide@vger.kernel.org > >>> Signed-off-by: Kees Cook <keescook@chromium.org> > >>> --- > >>> Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE > >>> improvements that are close to being finished. :) > >> > >> I don't maintain libata anymore, so Damien is the guy to nudge ;-) > > > > Ah-ha, okay, thanks. > > > > /me waves "hi" to Damien. :) > > Hi Kees, > > This is already queued up in libata tree for-5.17 branch. I have not > sent my PR to Linus yet as I am letting things soack a little longer in > for-next (for the various arch compile tests). Oh thank you! Sorry I missed the pull. I didn't see it in -next yet, so I assumed it hadn't been pulled anywhere. > Please check that branch to see if all is OK ! Found it: https://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git/log/?h=for-next Yup, looks good: https://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git/commit/?h=for-next&id=23c72ffedeed6d513144fa09834b1eb0cb2b7373 Thanks!
On 1/13/22 09:30, Kees Cook wrote: > On Thu, Jan 13, 2022 at 08:47:37AM +0900, Damien Le Moal wrote: >> On 1/13/22 08:15, Kees Cook wrote: >>> On Wed, Jan 12, 2022 at 03:23:40PM -0700, Jens Axboe wrote: >>>> On 1/12/22 3:06 PM, Kees Cook wrote: >>>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time >>>>> field bounds checking for memcpy(), memmove(), and memset(), avoid >>>>> intentionally writing across neighboring fields. >>>>> >>>>> Use struct_group() in struct command_desc around members acmd and fill, >>>>> so they can be referenced together. This will allow memset(), memcpy(), >>>>> and sizeof() to more easily reason about sizes, improve readability, >>>>> and avoid future warnings about writing beyond the end of acmd: >>>>> >>>>> In function 'fortify_memset_chk', >>>>> inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: >>>>> ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] >>>>> 199 | __write_overflow_field(); >>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~ >>>>> >>>>> Cc: Jens Axboe <axboe@kernel.dk> >>>>> Cc: linux-ide@vger.kernel.org >>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>> --- >>>>> Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE >>>>> improvements that are close to being finished. :) >>>> >>>> I don't maintain libata anymore, so Damien is the guy to nudge ;-) >>> >>> Ah-ha, okay, thanks. >>> >>> /me waves "hi" to Damien. :) >> >> Hi Kees, >> >> This is already queued up in libata tree for-5.17 branch. I have not >> sent my PR to Linus yet as I am letting things soack a little longer in >> for-next (for the various arch compile tests). > > Oh thank you! Sorry I missed the pull. I didn't see it in -next yet, so > I assumed it hadn't been pulled anywhere. Uh... Weird. That one has been in libata for-next since a while back. So it should be in linux-next. > >> Please check that branch to see if all is OK ! > > Found it: > https://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git/log/?h=for-next > > Yup, looks good: > https://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata.git/commit/?h=for-next&id=23c72ffedeed6d513144fa09834b1eb0cb2b7373 OK ! > > Thanks! >
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index 3b31a4f596d8..c5a2c1e9ed6b 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -246,8 +246,10 @@ enum { struct command_desc { u8 cfis[8 * 4]; u8 sfis[8 * 4]; - u8 acmd[4 * 4]; - u8 fill[4 * 4]; + struct_group(cdb, + u8 acmd[4 * 4]; + u8 fill[4 * 4]; + ); u32 prdt[SATA_FSL_MAX_PRD_DIRECT * 4]; u32 prdt_indirect[(SATA_FSL_MAX_PRD - SATA_FSL_MAX_PRD_DIRECT) * 4]; }; @@ -531,8 +533,8 @@ static enum ata_completion_errors sata_fsl_qc_prep(struct ata_queued_cmd *qc) /* setup "ACMD - atapi command" in cmd. desc. if this is ATAPI cmd */ if (ata_is_atapi(qc->tf.protocol)) { desc_info |= ATAPI_CMD; - memset((void *)&cd->acmd, 0, 32); - memcpy((void *)&cd->acmd, qc->cdb, qc->dev->cdb_len); + memset(&cd->cdb, 0, sizeof(cd->cdb)); + memcpy(&cd->cdb, qc->cdb, qc->dev->cdb_len); } if (qc->flags & ATA_QCFLAG_DMAMAP)
In preparation for FORTIFY_SOURCE performing compile-time and run-time field bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields. Use struct_group() in struct command_desc around members acmd and fill, so they can be referenced together. This will allow memset(), memcpy(), and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of acmd: In function 'fortify_memset_chk', inlined from 'sata_fsl_qc_prep' at drivers/ata/sata_fsl.c:534:3: ./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning] 199 | __write_overflow_field(); | ^~~~~~~~~~~~~~~~~~~~~~~~ Cc: Jens Axboe <axboe@kernel.dk> Cc: linux-ide@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- Jens, can you take (or Ack) this? It's a dependency for the FORTIFY_SOURCE improvements that are close to being finished. :) --- drivers/ata/sata_fsl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)