* [PATCH] sata_fsl: Use struct_group() for memcpy() region
@ 2021-11-18 18:38 Kees Cook
2021-11-18 23:17 ` Damien Le Moal
2021-11-29 11:37 ` Damien Le Moal
0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2021-11-18 18:38 UTC (permalink / raw)
To: Jens Axboe
Cc: Kees Cook, Damien Le Moal, linux-kernel, linux-ide, linux-hardening
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();
| ^~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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 e5838b23c9e0..fec3c9032606 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sata_fsl: Use struct_group() for memcpy() region
2021-11-18 18:38 [PATCH] sata_fsl: Use struct_group() for memcpy() region Kees Cook
@ 2021-11-18 23:17 ` Damien Le Moal
2021-11-18 23:39 ` Kees Cook
2021-11-29 11:37 ` Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2021-11-18 23:17 UTC (permalink / raw)
To: Kees Cook, Jens Axboe; +Cc: linux-kernel, linux-ide, linux-hardening
On 2021/11/19 3:38, 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();
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
This lacks some context with regard to FORTIFY_SOURCE and struct_group(). Is
that already in 5.16 ? It sounds like it is not. Do you want a ack ? Or do you
want me to queue this up for 5.17 ?
Cheers.
> ---
> 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 e5838b23c9e0..fec3c9032606 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)
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sata_fsl: Use struct_group() for memcpy() region
2021-11-18 23:17 ` Damien Le Moal
@ 2021-11-18 23:39 ` Kees Cook
2021-11-18 23:52 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2021-11-18 23:39 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Jens Axboe, linux-kernel, linux-ide, linux-hardening
On Fri, Nov 19, 2021 at 08:17:14AM +0900, Damien Le Moal wrote:
> On 2021/11/19 3:38, 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();
> > | ^~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> This lacks some context with regard to FORTIFY_SOURCE and struct_group(). Is
> that already in 5.16 ? It sounds like it is not. Do you want a ack ? Or do you
> want me to queue this up for 5.17 ?
Ah yes, some details are here in the earlier "big" series cover letter
here:
https://lore.kernel.org/linux-hardening/20210818060533.3569517-1-keescook@chromium.org/
One of the requests from earlier review was to split it up for separate
trees for the maintainers that wanted to take stuff via their trees
directly.
The new helpers are landed as of v5.16-rc1, so it can go either way, but
given that the merge window is closed, I would expect this to be for
v5.17.
I am happy to to carry it in my fortify topic branch that I'm expecting
to send for 5.17, but totally up to you. Some folks like to take these
changes via their trees, others would rather not be bothered with it. :)
Thanks!
-Kees
>
> Cheers.
>
> > ---
> > 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 e5838b23c9e0..fec3c9032606 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)
> >
>
>
> --
> Damien Le Moal
> Western Digital Research
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sata_fsl: Use struct_group() for memcpy() region
2021-11-18 23:39 ` Kees Cook
@ 2021-11-18 23:52 ` Damien Le Moal
2021-11-18 23:55 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2021-11-18 23:52 UTC (permalink / raw)
To: Kees Cook; +Cc: Jens Axboe, linux-kernel, linux-ide, linux-hardening
On 2021/11/19 8:39, Kees Cook wrote:
> On Fri, Nov 19, 2021 at 08:17:14AM +0900, Damien Le Moal wrote:
>> On 2021/11/19 3:38, 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();
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> This lacks some context with regard to FORTIFY_SOURCE and struct_group(). Is
>> that already in 5.16 ? It sounds like it is not. Do you want a ack ? Or do you
>> want me to queue this up for 5.17 ?
>
> Ah yes, some details are here in the earlier "big" series cover letter
> here:
> https://lore.kernel.org/linux-hardening/20210818060533.3569517-1-keescook@chromium.org/
>
> One of the requests from earlier review was to split it up for separate
> trees for the maintainers that wanted to take stuff via their trees
> directly.
>
> The new helpers are landed as of v5.16-rc1, so it can go either way, but
> given that the merge window is closed, I would expect this to be for
> v5.17.
>
> I am happy to to carry it in my fortify topic branch that I'm expecting
> to send for 5.17, but totally up to you. Some folks like to take these
> changes via their trees, others would rather not be bothered with it. :)
OK. Since it looks like the compilation warning will trigger only when your big
series land in 5.17, I will queue this in for-5.17 (still need to create than
one). Is it ok with you ?
>
> Thanks!
>
> -Kees
>
>>
>> Cheers.
>>
>>> ---
>>> 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 e5838b23c9e0..fec3c9032606 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)
>>>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sata_fsl: Use struct_group() for memcpy() region
2021-11-18 23:52 ` Damien Le Moal
@ 2021-11-18 23:55 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2021-11-18 23:55 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Jens Axboe, linux-kernel, linux-ide, linux-hardening
On Fri, Nov 19, 2021 at 08:52:36AM +0900, Damien Le Moal wrote:
> On 2021/11/19 8:39, Kees Cook wrote:
> > On Fri, Nov 19, 2021 at 08:17:14AM +0900, Damien Le Moal wrote:
> >> On 2021/11/19 3:38, 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();
> >>> | ^~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> Signed-off-by: Kees Cook <keescook@chromium.org>
> >>
> >> This lacks some context with regard to FORTIFY_SOURCE and struct_group(). Is
> >> that already in 5.16 ? It sounds like it is not. Do you want a ack ? Or do you
> >> want me to queue this up for 5.17 ?
> >
> > Ah yes, some details are here in the earlier "big" series cover letter
> > here:
> > https://lore.kernel.org/linux-hardening/20210818060533.3569517-1-keescook@chromium.org/
> >
> > One of the requests from earlier review was to split it up for separate
> > trees for the maintainers that wanted to take stuff via their trees
> > directly.
> >
> > The new helpers are landed as of v5.16-rc1, so it can go either way, but
> > given that the merge window is closed, I would expect this to be for
> > v5.17.
> >
> > I am happy to to carry it in my fortify topic branch that I'm expecting
> > to send for 5.17, but totally up to you. Some folks like to take these
> > changes via their trees, others would rather not be bothered with it. :)
>
> OK. Since it looks like the compilation warning will trigger only when your big
> series land in 5.17, I will queue this in for-5.17 (still need to create than
> one). Is it ok with you ?
Yup, that works for me.
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sata_fsl: Use struct_group() for memcpy() region
2021-11-18 18:38 [PATCH] sata_fsl: Use struct_group() for memcpy() region Kees Cook
2021-11-18 23:17 ` Damien Le Moal
@ 2021-11-29 11:37 ` Damien Le Moal
1 sibling, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2021-11-29 11:37 UTC (permalink / raw)
To: Kees Cook, Jens Axboe; +Cc: linux-kernel, linux-ide, linux-hardening
On 2021/11/19 3:38, 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();
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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 e5838b23c9e0..fec3c9032606 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)
>
Applied to for-5.17. Thanks.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-29 11:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 18:38 [PATCH] sata_fsl: Use struct_group() for memcpy() region Kees Cook
2021-11-18 23:17 ` Damien Le Moal
2021-11-18 23:39 ` Kees Cook
2021-11-18 23:52 ` Damien Le Moal
2021-11-18 23:55 ` Kees Cook
2021-11-29 11:37 ` Damien Le Moal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).