Hi all,

As part of the efforts to globally enable -Wflex-array-member-not-at-end,
I'm currently trying to fix the following warnings:

drivers/rtc/rtc-cros-ec.c:62:40: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/rtc/rtc-cros-ec.c:40:40: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

The issue is that `struct cros_ec_command` is a flexible structure (which
means that it contains a flexible-array member), and there is an object
of this type (msg) declared within another structure but at the end.

It seems that the following patch would suffice, as long as the flex-array
member in `struct cros_ec_command` is not expected to be accessed and
overlap with `struct ec_response_rtc data` in a "controlled manner":

diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
index 865c2e82c7a5..7e9bbab47e4c 100644
--- a/drivers/rtc/rtc-cros-ec.c
+++ b/drivers/rtc/rtc-cros-ec.c
@@ -37,8 +37,8 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
 {
        int ret;
        struct {
-               struct cros_ec_command msg;
                struct ec_response_rtc data;
+               struct cros_ec_command msg;
        } __packed msg;
 
        memset(&msg, 0, sizeof(msg));
@@ -59,8 +59,8 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
 {
        int ret;
        struct {
-               struct cros_ec_command msg;
                struct ec_response_rtc data;
+               struct cros_ec_command msg;
        } __packed msg;
 
        memset(&msg, 0, sizeof(msg));

Otherwise, we probably need to use struct_group_tagged() as follows:

diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
index 865c2e82c7a5..6dc815bdbcd9 100644
--- a/drivers/rtc/rtc-cros-ec.c
+++ b/drivers/rtc/rtc-cros-ec.c
@@ -37,7 +37,7 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
 {
        int ret;
        struct {
-               struct cros_ec_command msg;
+               struct cros_ec_command_hdr msg;
                struct ec_response_rtc data;
        } __packed msg;
 
@@ -45,7 +45,10 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
        msg.msg.command = command;
        msg.msg.insize = sizeof(msg.data);
 
-       ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+       ret = cros_ec_cmd_xfer_status(cros_ec,
+                                     container_of(&msg.msg,
+                                                  struct cros_ec_command,
+                                                  __hdr));
        if (ret < 0)
                return ret;
 
@@ -59,7 +62,7 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
 {
        int ret;
        struct {
-               struct cros_ec_command msg;
+               struct cros_ec_command_hdr msg;
                struct ec_response_rtc data;
        } __packed msg;
 
@@ -68,7 +71,10 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
        msg.msg.outsize = sizeof(msg.data);
        msg.data.time = param;
 
-       ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+       ret = cros_ec_cmd_xfer_status(cros_ec,
+                                     container_of(&msg.msg,
+                                                  struct cros_ec_command,
+                                                  __hdr));
        if (ret < 0)
                return ret;
        return 0;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 3ec24f445c29..2a638c8c5ec2 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -80,11 +80,13 @@ enum {
  * @data: Where to put the incoming data from EC and outgoing data to EC.
  */
 struct cros_ec_command {
-       uint32_t version;
-       uint32_t command;
-       uint32_t outsize;
-       uint32_t insize;
-       uint32_t result;
+       struct_group_tagged(cros_ec_command_hdr, __hdr,
+               uint32_t version;
+               uint32_t command;
+               uint32_t outsize;
+               uint32_t insize;
+               uint32_t result;
+       );
        uint8_t data[];
 };

What do you think?

Thanks!
--
Gustavo

On Mon, Mar 10, 2025 at 03:46:26PM +1030, Gustavo A. R. Silva wrote:
> Hi all,
> 
> As part of the efforts to globally enable -Wflex-array-member-not-at-end,
> I'm currently trying to fix the following warnings:
> 
> drivers/rtc/rtc-cros-ec.c:62:40: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> drivers/rtc/rtc-cros-ec.c:40:40: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> The issue is that `struct cros_ec_command` is a flexible structure (which
> means that it contains a flexible-array member), and there is an object
> of this type (msg) declared within another structure but at the end.
                                                          ^ not

> It seems that the following patch would suffice, as long as the flex-array
> member in `struct cros_ec_command` is not expected to be accessed and
> overlap with `struct ec_response_rtc data` in a "controlled manner":
> 
> diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
> index 865c2e82c7a5..7e9bbab47e4c 100644
> --- a/drivers/rtc/rtc-cros-ec.c
> +++ b/drivers/rtc/rtc-cros-ec.c
> @@ -37,8 +37,8 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
>  {
>         int ret;
>         struct {
> -               struct cros_ec_command msg;
>                 struct ec_response_rtc data;
> +               struct cros_ec_command msg;
>         } __packed msg;
>  
>         memset(&msg, 0, sizeof(msg));
> @@ -59,8 +59,8 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
>  {
>         int ret;
>         struct {
> -               struct cros_ec_command msg;
>                 struct ec_response_rtc data;
> +               struct cros_ec_command msg;
>         } __packed msg;
>  
>         memset(&msg, 0, sizeof(msg));

This doesn't work.  The header (i.e. struct cros_ec_command) is expected to
allocate right before the payload (e.g. struct ec_response_rtc)[1][2].

[1]: https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/platform/chrome/cros_ec_proto.c#L82
[2]: https://elixir.bootlin.com/linux/v6.14-rc1/source/drivers/platform/chrome/cros_ec_i2c.c#L166

> Otherwise, we probably need to use struct_group_tagged() as follows:

I'm not a big fan of the solution.  How about using something similar to:

struct cros_ec_command *msg = kzalloc(sizeof(*msg) +
                                      sizeof(struct ec_response_rtc), ...);

> I'm not a big fan of the solution.  How about using something similar to:
> 
> struct cros_ec_command *msg = kzalloc(sizeof(*msg) +
>                                        sizeof(struct ec_response_rtc), ...);
> 

Thanks for the feedback. :) We can actually use DEFINE_RAW_FLEX():

https://lore.kernel.org/linux-hardening/Z9N8BsVJF-s6Hcvd@kspp/

--
Gustavo