linux-kernel - Re: [PATCH 2/3] firmware: arm_scmi: Remove unneeded _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8683b876-72b9-2c94-12f6-9b9456627a7d@arm.com>
Date:   Mon, 13 Jul 2020 12:18:00 +0100
From:   Steven Price <steven.price@....com>
To:     Cristian Marussi <cristian.marussi@....com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     arnd@...db.de, sudeep.holla@....com
Subject: Re: [PATCH 2/3] firmware: arm_scmi: Remove unneeded __packed
 attribute

On 10/07/2020 14:39, Cristian Marussi wrote:
> Remove __packed attribute from struct scmi_event_header.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>

A drive-by review. But this doesn't look safe to me. sizeof(struct 
scmi_event_header) is used in several places and this change will modify 
that from 13 to 16, but leave the structure members at the same offset 
(as the members are naturally aligned). In particular the naïve header 
size is now bigger than the offset to payld.

What is the justification for __packed being 'unneeded'? Perhaps there's 
some reason in the code why this doesn't matter, but if so I feel this 
should be included in the commit message.

Steve

> ---
>   drivers/firmware/arm_scmi/notify.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index c4d006cfde88..752415367305 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -258,7 +258,7 @@ struct scmi_event_header {
>   	u8	evt_id;
>   	size_t	payld_sz;
>   	u8	payld[];
> -} __packed;
> +};
>   
>   struct scmi_registered_event;
>   
>