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@xxxxxxxxxxxx>
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
