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 <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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(-)
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)
--
2.30.2
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 <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 ;-)
--
Jens Axboe
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 <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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. :)
--
Kees Cook
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 <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Kees Cook <[email protected]>
>>> ---
>>> 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.
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> >>> Cc: [email protected]
> >>> Signed-off-by: Kees Cook <[email protected]>
> >>> ---
> >>> 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!
--
Kees Cook
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 <[email protected]>
>>>>> Cc: [email protected]
>>>>> Signed-off-by: Kees Cook <[email protected]>
>>>>> ---
>>>>> 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!
>
--
Damien Le Moal
Western Digital Research