* [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
@ 2024-03-25 18:03 Gustavo A. R. Silva
2024-04-29 16:38 ` Kees Cook
0 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2024-03-25 18:03 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Mathieu Poirier
Cc: linux-arm-msm, linux-remoteproc, linux-kernel,
Gustavo A. R. Silva, linux-hardening
-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[];
};
@@ -455,12 +458,9 @@ static void qcom_glink_intent_req_abort(struct glink_channel *channel)
static int qcom_glink_send_open_req(struct qcom_glink *glink,
struct glink_channel *channel)
{
- struct {
- struct glink_msg msg;
- u8 name[GLINK_NAME_SIZE];
- } __packed req;
+ DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
int name_len = strlen(channel->name) + 1;
- int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
+ int req_len = ALIGN(sizeof(*req) + name_len, 8);
int ret;
unsigned long flags;
@@ -476,10 +476,10 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
channel->lcid = ret;
- req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
- req.msg.param1 = cpu_to_le16(channel->lcid);
- req.msg.param2 = cpu_to_le32(name_len);
- strcpy(req.name, channel->name);
+ req->cmd = cpu_to_le16(GLINK_CMD_OPEN);
+ req->param1 = cpu_to_le16(channel->lcid);
+ req->param2 = cpu_to_le32(name_len);
+ strcpy(req->data, channel->name);
ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
if (ret)
@@ -826,7 +826,9 @@ static int qcom_glink_rx_defer(struct qcom_glink *glink, size_t extra)
INIT_LIST_HEAD(&dcmd->node);
- qcom_glink_rx_peek(glink, &dcmd->msg, 0, sizeof(dcmd->msg) + extra);
+ qcom_glink_rx_peek(glink,
+ container_of(&dcmd->msg, struct glink_msg, hdr), 0,
+ sizeof(dcmd->msg) + extra);
spin_lock(&glink->rx_lock);
list_add_tail(&dcmd->node, &glink->rx_queue);
@@ -843,7 +845,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
struct glink_core_rx_intent *intent;
struct glink_channel *channel;
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
__le32 chunk_size;
__le32 left_size;
} __packed hdr;
@@ -965,7 +967,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
};
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
struct intent_pair intents[];
} __packed * msg;
@@ -1377,7 +1379,7 @@ static int __qcom_glink_send(struct glink_channel *channel,
struct glink_core_rx_intent *tmp;
int iid = 0;
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
__le32 chunk_size;
__le32 left_size;
} __packed req;
@@ -1685,7 +1687,7 @@ static void qcom_glink_work(struct work_struct *work)
list_del(&dcmd->node);
spin_unlock_irqrestore(&glink->rx_lock, flags);
- msg = &dcmd->msg;
+ msg = container_of(&dcmd->msg, struct glink_msg, hdr);
cmd = le16_to_cpu(msg->cmd);
param1 = le16_to_cpu(msg->param1);
param2 = le32_to_cpu(msg->param2);
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-03-25 18:03 Gustavo A. R. Silva
@ 2024-04-29 16:38 ` Kees Cook
2024-08-07 20:43 ` Gustavo A. R. Silva
0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2024-04-29 16:38 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm,
linux-remoteproc, linux-kernel, linux-hardening
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
@ 2024-08-07 15:19 Gustavo A. R. Silva
2024-08-08 18:51 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2024-08-07 15:19 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier
Cc: linux-arm-msm, linux-remoteproc, linux-kernel,
Gustavo A. R. Silva, linux-hardening
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.
So, in order to avoid ending up with a flexible-array member in the
middle of multiple other structs, we use the `__struct_group()`
helper to create a new tagged `struct glink_msg_hdr`. This structure
groups together all the members of the flexible `struct glink_msg`
except the flexible array.
As a result, the array is effectively separated from the rest of the
members without modifying the memory layout of the flexible structure.
We then change the type of the middle struct members currently causing
trouble from `struct glink_msg` to `struct glink_msg_hdr`.
We also want to ensure that when new members need to be added to the
flexible structure, they are always included within the newly created
tagged struct. For this, we use `static_assert()`. This ensures that the
memory layout for both the flexible structure and the new tagged struct
is the same after any changes.
This approach avoids having to implement `struct glink_msg_hdr` as a
completely separate structure, thus preventing having to maintain two
independent but basically identical structures, closing the door to
potential bugs in the future.
We also use `container_of()` whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible-array
member, if necessary.
Additionally, we use the `DEFINE_RAW_FLEX()` helper for an on-stack
definition of a flexible structure where the size for 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 | 42 +++++++++++++++++--------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 82d460ff4777..ed89b810f262 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -30,11 +30,16 @@
#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;
+static_assert(offsetof(struct glink_msg, data) == sizeof(struct glink_msg_hdr),
+ "struct member likely outside of __struct_group()");
/**
* struct glink_defer_cmd - deferred incoming control message
@@ -48,7 +53,7 @@ struct glink_msg {
struct glink_defer_cmd {
struct list_head node;
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
u8 data[];
};
@@ -455,12 +460,9 @@ static void qcom_glink_intent_req_abort(struct glink_channel *channel)
static int qcom_glink_send_open_req(struct qcom_glink *glink,
struct glink_channel *channel)
{
- struct {
- struct glink_msg msg;
- u8 name[GLINK_NAME_SIZE];
- } __packed req;
+ DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
int name_len = strlen(channel->name) + 1;
- int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
+ int req_len = ALIGN(sizeof(*req) + name_len, 8);
int ret;
unsigned long flags;
@@ -476,12 +478,12 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
channel->lcid = ret;
- req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
- req.msg.param1 = cpu_to_le16(channel->lcid);
- req.msg.param2 = cpu_to_le32(name_len);
- strcpy(req.name, channel->name);
+ req->cmd = cpu_to_le16(GLINK_CMD_OPEN);
+ req->param1 = cpu_to_le16(channel->lcid);
+ req->param2 = cpu_to_le32(name_len);
+ strcpy(req->data, channel->name);
- ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
+ ret = qcom_glink_tx(glink, req, req_len, NULL, 0, true);
if (ret)
goto remove_idr;
@@ -826,7 +828,9 @@ static int qcom_glink_rx_defer(struct qcom_glink *glink, size_t extra)
INIT_LIST_HEAD(&dcmd->node);
- qcom_glink_rx_peek(glink, &dcmd->msg, 0, sizeof(dcmd->msg) + extra);
+ qcom_glink_rx_peek(glink,
+ container_of(&dcmd->msg, struct glink_msg, hdr), 0,
+ sizeof(dcmd->msg) + extra);
spin_lock(&glink->rx_lock);
list_add_tail(&dcmd->node, &glink->rx_queue);
@@ -843,7 +847,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
struct glink_core_rx_intent *intent;
struct glink_channel *channel;
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
__le32 chunk_size;
__le32 left_size;
} __packed hdr;
@@ -965,7 +969,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
};
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
struct intent_pair intents[];
} __packed * msg;
@@ -1377,7 +1381,7 @@ static int __qcom_glink_send(struct glink_channel *channel,
struct glink_core_rx_intent *tmp;
int iid = 0;
struct {
- struct glink_msg msg;
+ struct glink_msg_hdr msg;
__le32 chunk_size;
__le32 left_size;
} __packed req;
@@ -1685,7 +1689,7 @@ static void qcom_glink_work(struct work_struct *work)
list_del(&dcmd->node);
spin_unlock_irqrestore(&glink->rx_lock, flags);
- msg = &dcmd->msg;
+ msg = container_of(&dcmd->msg, struct glink_msg, hdr);
cmd = le16_to_cpu(msg->cmd);
param1 = le16_to_cpu(msg->param1);
param2 = le32_to_cpu(msg->param2);
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-04-29 16:38 ` Kees Cook
@ 2024-08-07 20:43 ` Gustavo A. R. Silva
0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2024-08-07 20:43 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm,
linux-remoteproc, linux-kernel, linux-hardening
>> @@ -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.:
Whaa.. I'm sorry, I totally missed this response. I think I was traveling
a lot back then.
>
> - 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.
Yes; I think I can change the code like this. :)
Thanks!
--
Gustavo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-08-07 15:19 [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
@ 2024-08-08 18:51 ` Kees Cook
2024-08-19 19:45 ` Gustavo A. R. Silva
2024-09-13 8:10 ` Gustavo A. R. Silva
2024-09-13 21:16 ` Bjorn Andersson
2 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2024-08-08 18:51 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Bjorn Andersson, Mathieu Poirier, linux-arm-msm, linux-remoteproc,
linux-kernel, linux-hardening
On Wed, Aug 07, 2024 at 09:19:07AM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `__struct_group()`
> helper to create a new tagged `struct glink_msg_hdr`. This structure
> groups together all the members of the flexible `struct glink_msg`
> except the flexible array.
>
> As a result, the array is effectively separated from the rest of the
> members without modifying the memory layout of the flexible structure.
> We then change the type of the middle struct members currently causing
> trouble from `struct glink_msg` to `struct glink_msg_hdr`.
>
> We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes.
>
> This approach avoids having to implement `struct glink_msg_hdr` as a
> completely separate structure, thus preventing having to maintain two
> independent but basically identical structures, closing the door to
> potential bugs in the future.
>
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which we can access the flexible-array
> member, if necessary.
>
> Additionally, we use the `DEFINE_RAW_FLEX()` helper for an on-stack
> definition of a flexible structure where the size for 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>
Looks correct to me. As a separate change, I wonder if the strcpy()
should be replaced with strscpy_pad(), but I think it's all okay as-is,
since channel->name seems to be set from another fixed-size array that
is the same size.
Reviewed-by: Kees Cook <kees@kernel.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-08-08 18:51 ` Kees Cook
@ 2024-08-19 19:45 ` Gustavo A. R. Silva
0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2024-08-19 19:45 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva
Cc: Bjorn Andersson, Mathieu Poirier, linux-arm-msm, linux-remoteproc,
linux-kernel, linux-hardening
On 08/08/24 12:51, Kees Cook wrote:
> On Wed, Aug 07, 2024 at 09:19:07AM -0600, Gustavo A. R. Silva wrote:
>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>> getting ready to enable it, globally.
>>
>> So, in order to avoid ending up with a flexible-array member in the
>> middle of multiple other structs, we use the `__struct_group()`
>> helper to create a new tagged `struct glink_msg_hdr`. This structure
>> groups together all the members of the flexible `struct glink_msg`
>> except the flexible array.
>>
>> As a result, the array is effectively separated from the rest of the
>> members without modifying the memory layout of the flexible structure.
>> We then change the type of the middle struct members currently causing
>> trouble from `struct glink_msg` to `struct glink_msg_hdr`.
>>
>> We also want to ensure that when new members need to be added to the
>> flexible structure, they are always included within the newly created
>> tagged struct. For this, we use `static_assert()`. This ensures that the
>> memory layout for both the flexible structure and the new tagged struct
>> is the same after any changes.
>>
>> This approach avoids having to implement `struct glink_msg_hdr` as a
>> completely separate structure, thus preventing having to maintain two
>> independent but basically identical structures, closing the door to
>> potential bugs in the future.
>>
>> We also use `container_of()` whenever we need to retrieve a pointer to
>> the flexible structure, through which we can access the flexible-array
>> member, if necessary.
>>
>> Additionally, we use the `DEFINE_RAW_FLEX()` helper for an on-stack
>> definition of a flexible structure where the size for 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>
>
> Looks correct to me. As a separate change, I wonder if the strcpy()
> should be replaced with strscpy_pad(), but I think it's all okay as-is,
> since channel->name seems to be set from another fixed-size array that
> is the same size.
Yes, I noticed the same after sending the patch. :p
>
> Reviewed-by: Kees Cook <kees@kernel.org>
>
Thanks!
--
Gustavo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-08-07 15:19 [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
2024-08-08 18:51 ` Kees Cook
@ 2024-09-13 8:10 ` Gustavo A. R. Silva
2024-09-13 21:16 ` Bjorn Andersson
2 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2024-09-13 8:10 UTC (permalink / raw)
To: Gustavo A. R. Silva, Bjorn Andersson, Mathieu Poirier
Cc: linux-arm-msm, linux-remoteproc, linux-kernel, linux-hardening
Hi all,
Friendly ping: who can take this, please? 🙂
Thanks
-Gustavo
On 07/08/24 17:19, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `__struct_group()`
> helper to create a new tagged `struct glink_msg_hdr`. This structure
> groups together all the members of the flexible `struct glink_msg`
> except the flexible array.
>
> As a result, the array is effectively separated from the rest of the
> members without modifying the memory layout of the flexible structure.
> We then change the type of the middle struct members currently causing
> trouble from `struct glink_msg` to `struct glink_msg_hdr`.
>
> We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes.
>
> This approach avoids having to implement `struct glink_msg_hdr` as a
> completely separate structure, thus preventing having to maintain two
> independent but basically identical structures, closing the door to
> potential bugs in the future.
>
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which we can access the flexible-array
> member, if necessary.
>
> Additionally, we use the `DEFINE_RAW_FLEX()` helper for an on-stack
> definition of a flexible structure where the size for 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 | 42 +++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 82d460ff4777..ed89b810f262 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -30,11 +30,16 @@
> #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;
> +static_assert(offsetof(struct glink_msg, data) == sizeof(struct glink_msg_hdr),
> + "struct member likely outside of __struct_group()");
>
> /**
> * struct glink_defer_cmd - deferred incoming control message
> @@ -48,7 +53,7 @@ struct glink_msg {
> struct glink_defer_cmd {
> struct list_head node;
>
> - struct glink_msg msg;
> + struct glink_msg_hdr msg;
> u8 data[];
> };
>
> @@ -455,12 +460,9 @@ static void qcom_glink_intent_req_abort(struct glink_channel *channel)
> static int qcom_glink_send_open_req(struct qcom_glink *glink,
> struct glink_channel *channel)
> {
> - struct {
> - struct glink_msg msg;
> - u8 name[GLINK_NAME_SIZE];
> - } __packed req;
> + DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
> int name_len = strlen(channel->name) + 1;
> - int req_len = ALIGN(sizeof(req.msg) + name_len, 8);
> + int req_len = ALIGN(sizeof(*req) + name_len, 8);
> int ret;
> unsigned long flags;
>
> @@ -476,12 +478,12 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
>
> channel->lcid = ret;
>
> - req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
> - req.msg.param1 = cpu_to_le16(channel->lcid);
> - req.msg.param2 = cpu_to_le32(name_len);
> - strcpy(req.name, channel->name);
> + req->cmd = cpu_to_le16(GLINK_CMD_OPEN);
> + req->param1 = cpu_to_le16(channel->lcid);
> + req->param2 = cpu_to_le32(name_len);
> + strcpy(req->data, channel->name);
>
> - ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
> + ret = qcom_glink_tx(glink, req, req_len, NULL, 0, true);
> if (ret)
> goto remove_idr;
>
> @@ -826,7 +828,9 @@ static int qcom_glink_rx_defer(struct qcom_glink *glink, size_t extra)
>
> INIT_LIST_HEAD(&dcmd->node);
>
> - qcom_glink_rx_peek(glink, &dcmd->msg, 0, sizeof(dcmd->msg) + extra);
> + qcom_glink_rx_peek(glink,
> + container_of(&dcmd->msg, struct glink_msg, hdr), 0,
> + sizeof(dcmd->msg) + extra);
>
> spin_lock(&glink->rx_lock);
> list_add_tail(&dcmd->node, &glink->rx_queue);
> @@ -843,7 +847,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
> struct glink_core_rx_intent *intent;
> struct glink_channel *channel;
> struct {
> - struct glink_msg msg;
> + struct glink_msg_hdr msg;
> __le32 chunk_size;
> __le32 left_size;
> } __packed hdr;
> @@ -965,7 +969,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink,
> };
>
> struct {
> - struct glink_msg msg;
> + struct glink_msg_hdr msg;
> struct intent_pair intents[];
> } __packed * msg;
>
> @@ -1377,7 +1381,7 @@ static int __qcom_glink_send(struct glink_channel *channel,
> struct glink_core_rx_intent *tmp;
> int iid = 0;
> struct {
> - struct glink_msg msg;
> + struct glink_msg_hdr msg;
> __le32 chunk_size;
> __le32 left_size;
> } __packed req;
> @@ -1685,7 +1689,7 @@ static void qcom_glink_work(struct work_struct *work)
> list_del(&dcmd->node);
> spin_unlock_irqrestore(&glink->rx_lock, flags);
>
> - msg = &dcmd->msg;
> + msg = container_of(&dcmd->msg, struct glink_msg, hdr);
> cmd = le16_to_cpu(msg->cmd);
> param1 = le16_to_cpu(msg->param1);
> param2 = le32_to_cpu(msg->param2);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
2024-08-07 15:19 [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
2024-08-08 18:51 ` Kees Cook
2024-09-13 8:10 ` Gustavo A. R. Silva
@ 2024-09-13 21:16 ` Bjorn Andersson
2 siblings, 0 replies; 8+ messages in thread
From: Bjorn Andersson @ 2024-09-13 21:16 UTC (permalink / raw)
To: Mathieu Poirier, Gustavo A. R. Silva
Cc: linux-arm-msm, linux-remoteproc, linux-kernel, linux-hardening
On Wed, 07 Aug 2024 09:19:07 -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
>
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `__struct_group()`
> helper to create a new tagged `struct glink_msg_hdr`. This structure
> groups together all the members of the flexible `struct glink_msg`
> except the flexible array.
>
> [...]
Applied, thanks!
[1/1] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings
commit: c1ddb29709e675ea2a406e3114dbf5c8c705dd59
Best regards,
--
Bjorn Andersson <andersson@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-09-13 21:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-07 15:19 [PATCH][next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings Gustavo A. R. Silva
2024-08-08 18:51 ` Kees Cook
2024-08-19 19:45 ` Gustavo A. R. Silva
2024-09-13 8:10 ` Gustavo A. R. Silva
2024-09-13 21:16 ` Bjorn Andersson
-- strict thread matches above, loose matches on Subject: below --
2024-03-25 18:03 Gustavo A. R. Silva
2024-04-29 16:38 ` Kees Cook
2024-08-07 20:43 ` Gustavo A. R. Silva
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 read-only IMAP folder(s) and NNTP newsgroup(s).