Received-SPF: pass (google.com: domain of linux-kernel+bounces-162697-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 29 Apr 2024 09:38:31 -0700
From: Kees Cook <keescook@chromium.org>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end
 warnings
Message-ID: <202404290937.C22567C1@keescook>
References: <ZgG8bbEzhmX5nGRE@neat>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZgG8bbEzhmX5nGRE@neat>

On Mon, Mar 25, 2024 at 12:03:25PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
> 
> There is currently an object (`msg`) in multiple structures that
> contains a flexible structure (`struct glink_msg`), for example:
> 
> struct glink_defer_cmd {
>         ...
> 
>         struct glink_msg msg;
>         u8 data[];
> };
> 
> So, in order to avoid ending up with a flexible-array member in the
> middle of another structure, we use the `__struct_group()` helper
> to separate the flexible array from the rest of the members in the
> flexible structure:
> 
> struct glink_msg {
>         __struct_group(glink_msg_hdr, hdr, __packed,
> 
>         ... the rest of members
> 
>         );
>         u8 data[];
> } __packed;
> 
> With the change described above, we now declare objects of the type of
> the tagged struct, in this case `struct glink_msg_hdr`, without
> embedding flexible arrays in the middle of other structures:
> 
> struct glink_defer_cmd {
>         ...
> 
>         struct glink_msg_hdr msg;
>         u8 data[];
> };
> 
> Also, use `container_of()` to retrieve a pointer to the flexible structure.
> 
> We also use the `DEFINE_RAW_FLEX()` helper for an in-stack definition of
> a flexible structure where the size of the flexible-array member is known
> at compile-time.
> 
> So, with these changes, fix the following warnings:
> drivers/rpmsg/qcom_glink_native.c:51:26: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:459:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:846:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:968:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rpmsg/qcom_glink_native.c:1380:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 38 ++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 82d460ff4777..878e3461bce1 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -30,9 +30,12 @@
>  #define RPM_GLINK_CID_MAX	65536
>  
>  struct glink_msg {
> -	__le16 cmd;
> -	__le16 param1;
> -	__le32 param2;
> +	/* New members must be added within the __struct_group() macro below. */
> +	__struct_group(glink_msg_hdr, hdr, __packed,
> +		__le16 cmd;
> +		__le16 param1;
> +		__le32 param2;
> +	);
>  	u8 data[];
>  } __packed;
>  
> @@ -48,7 +51,7 @@ struct glink_msg {
>  struct glink_defer_cmd {
>  	struct list_head node;
>  
> -	struct glink_msg msg;
> +	struct glink_msg_hdr msg;
>  	u8 data[];
>  };

Instead of this change (and the container_of() uses below), I think you
can just simply drop "data" here. I don't see anything using it except
the struct_size()s which can all change their "data" argument to
msg.data. e.g.:

-       dcmd = kzalloc(struct_size(dcmd, data, extra), GFP_ATOMIC);
+       dcmd = kzalloc(struct_size(dcmd, msg.data, extra), GFP_ATOMIC);

With those changed, I think this patch becomes more readable.

-Kees

-- 
Kees Cook