diff mbox series

[RESEND] sata_fsl: Use struct_group() for memcpy() region

Message ID 20220112220652.3952944-1-keescook@chromium.org (mailing list archive)
State Mainlined
Commit 23c72ffedeed6d513144fa09834b1eb0cb2b7373
Headers show
Series [RESEND] sata_fsl: Use struct_group() for memcpy() region | expand

Commit Message

Kees Cook Jan. 12, 2022, 10:06 p.m. UTC
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(-)

Comments

Jens Axboe Jan. 12, 2022, 10:23 p.m. UTC | #1
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 ;-)
Kees Cook Jan. 12, 2022, 11:15 p.m. UTC | #2
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. :)
Damien Le Moal Jan. 12, 2022, 11:47 p.m. UTC | #3
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.
Kees Cook Jan. 13, 2022, 12:30 a.m. UTC | #4
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!
Damien Le Moal Jan. 13, 2022, 12:34 a.m. UTC | #5
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 mbox series

Patch

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)