Re: [PATCH net-next v2] netlink: split up copies in the ack construction

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Kees Cook]

[sorry for the dup; resending with Gustavo actually CCed]

On Mon, Nov 14, 2022 at 09:06:14AM -0800, Jakub Kicinski wrote:
> On Sun, 13 Nov 2022 19:39:27 -0700 David Ahern wrote:
> > On Thu, Oct 27, 2022 at 02:25:53PM -0700, Jakub Kicinski wrote:
> > > diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> > > index e2ae82e3f9f7..5da0da59bf01 100644
> > > --- a/include/uapi/linux/netlink.h
> > > +++ b/include/uapi/linux/netlink.h
> > > @@ -48,6 +48,7 @@ struct sockaddr_nl {
> > >   * @nlmsg_flags: Additional flags
> > >   * @nlmsg_seq:   Sequence number
> > >   * @nlmsg_pid:   Sending process port ID
> > > + * @nlmsg_data:  Message payload
> > >   */
> > >  struct nlmsghdr {
> > >  	__u32		nlmsg_len;
> > > @@ -55,6 +56,7 @@ struct nlmsghdr {
> > >  	__u16		nlmsg_flags;
> > >  	__u32		nlmsg_seq;
> > >  	__u32		nlmsg_pid;
> > > +	__u8		nlmsg_data[];  
> > 
> > This breaks compile of iproute2 with clang. It does not like the
> > variable length array in the middle of a struct. While I could re-do the
> > structs in iproute2, I doubt it is alone in being affected by this
> > change.

Eww.

> 
> Kees, would you mind lending your expertise?
> 
> Not sure why something like (simplified):
> 
> struct top {
> 	struct nlmsghdr hdr;
> 	int tail;
> }; 
> 
> generates a warning:
> 
> In file included from stat-mr.c:7:
> In file included from ./res.h:9:
> In file included from ./rdma.h:21:
> In file included from ../include/utils.h:17:
> ../include/libnetlink.h:41:18: warning: field 'nlh' with variable sized type 'struct nlmsghdr' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>         struct nlmsghdr nlh;
>                         ^
> 
> which is not confined to -Wpedantic. 
> Seems like a useless warning :S

Yeah, this surprises me. But I can certainly reproduce it:
https://godbolt.org/z/fczq8sqbv

Gustavo, do you know what's happening here? GCC (and Clang) get mad
about doing this in the same struct:

struct nlmsghdr {
    __u32           nlmsg_len;
    __u16           nlmsg_flags;
    __u32           nlmsg_seq;
    __u32           nlmsg_pid;
    __u8            nlmsg_data[]; 
    int wat; 
};

<source>:10:21: error: flexible array member not at end of struct
   10 |     __u8            nlmsg_data[];
      |                     ^~~~~~~~~~

But the overlapping with other composite structs has been used in other
areas, I thought? (When I looked at this last, I thought the types just
had to overlap, but that doesn't seem to help here.)

-Kees

-- 
Kees Cook

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Kees Cook]

On Wed, Nov 16, 2022 at 02:56:25PM -0800, Kees Cook wrote:
> On Mon, Nov 14, 2022 at 09:06:14AM -0800, Jakub Kicinski wrote:
> > On Sun, 13 Nov 2022 19:39:27 -0700 David Ahern wrote:
> > > On Thu, Oct 27, 2022 at 02:25:53PM -0700, Jakub Kicinski wrote:
> > > > diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> > > > index e2ae82e3f9f7..5da0da59bf01 100644
> > > > --- a/include/uapi/linux/netlink.h
> > > > +++ b/include/uapi/linux/netlink.h
> > > > @@ -48,6 +48,7 @@ struct sockaddr_nl {
> > > >   * @nlmsg_flags: Additional flags
> > > >   * @nlmsg_seq:   Sequence number
> > > >   * @nlmsg_pid:   Sending process port ID
> > > > + * @nlmsg_data:  Message payload
> > > >   */
> > > >  struct nlmsghdr {
> > > >  	__u32		nlmsg_len;
> > > > @@ -55,6 +56,7 @@ struct nlmsghdr {
> > > >  	__u16		nlmsg_flags;
> > > >  	__u32		nlmsg_seq;
> > > >  	__u32		nlmsg_pid;
> > > > +	__u8		nlmsg_data[];  
> > > 
> > > This breaks compile of iproute2 with clang. It does not like the
> > > variable length array in the middle of a struct. While I could re-do the
> > > structs in iproute2, I doubt it is alone in being affected by this
> > > change.
> 
> Eww.
> 
> > 
> > Kees, would you mind lending your expertise?

Perhaps this would be better? We could leave the _header_ struct alone,
but add the data to the nlmsgerr struct instead?

diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 5da0da59bf01..d0629cb343b2 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -48,7 +48,6 @@ struct sockaddr_nl {
  * @nlmsg_flags: Additional flags
  * @nlmsg_seq:   Sequence number
  * @nlmsg_pid:   Sending process port ID
- * @nlmsg_data:  Message payload
  */
 struct nlmsghdr {
 	__u32		nlmsg_len;
@@ -56,7 +55,6 @@ struct nlmsghdr {
 	__u16		nlmsg_flags;
 	__u32		nlmsg_seq;
 	__u32		nlmsg_pid;
-	__u8		nlmsg_data[];
 };
 
 /* Flags values */
@@ -121,6 +119,7 @@ struct nlmsghdr {
 struct nlmsgerr {
 	int		error;
 	struct nlmsghdr msg;
+	__u8		data[];
 	/*
 	 * followed by the message contents unless NETLINK_CAP_ACK was set
 	 * or the ACK indicates success (error == 0)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index b8afec32cff6..fe8493d3ae56 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2514,8 +2514,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		if (!nlmsg_append(skb, nlmsg_len(nlh)))
 			goto err_bad_put;
 
-		memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
-		       nlmsg_len(nlh));
+		memcpy(errmsg->data, nlmsg_data(nlh), nlmsg_len(nlh));
 	}
 
 	if (tlvlen)


-- 
Kees Cook

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Gustavo A. R. Silva]

On 11/16/22 18:27, Kees Cook wrote:
> On Wed, Nov 16, 2022 at 02:56:25PM -0800, Kees Cook wrote:
>> On Mon, Nov 14, 2022 at 09:06:14AM -0800, Jakub Kicinski wrote:
>>> On Sun, 13 Nov 2022 19:39:27 -0700 David Ahern wrote:
>>>> On Thu, Oct 27, 2022 at 02:25:53PM -0700, Jakub Kicinski wrote:
>>>>> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
>>>>> index e2ae82e3f9f7..5da0da59bf01 100644
>>>>> --- a/include/uapi/linux/netlink.h
>>>>> +++ b/include/uapi/linux/netlink.h
>>>>> @@ -48,6 +48,7 @@ struct sockaddr_nl {
>>>>>    * @nlmsg_flags: Additional flags
>>>>>    * @nlmsg_seq:   Sequence number
>>>>>    * @nlmsg_pid:   Sending process port ID
>>>>> + * @nlmsg_data:  Message payload
>>>>>    */
>>>>>   struct nlmsghdr {
>>>>>   	__u32		nlmsg_len;
>>>>> @@ -55,6 +56,7 @@ struct nlmsghdr {
>>>>>   	__u16		nlmsg_flags;
>>>>>   	__u32		nlmsg_seq;
>>>>>   	__u32		nlmsg_pid;
>>>>> +	__u8		nlmsg_data[];
>>>>
>>>> This breaks compile of iproute2 with clang. It does not like the
>>>> variable length array in the middle of a struct. While I could re-do the
>>>> structs in iproute2, I doubt it is alone in being affected by this
>>>> change.

Yep; this is a fine Clang warning. We cannot have a variable length object
immediately followed by another object because that would cause undefined
behavior.

>>
>> Eww.
>>
>>>
>>> Kees, would you mind lending your expertise?
> 
> Perhaps this would be better? We could leave the _header_ struct alone,
> but add the data to the nlmsgerr struct instead?
> 
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 5da0da59bf01..d0629cb343b2 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -48,7 +48,6 @@ struct sockaddr_nl {
>    * @nlmsg_flags: Additional flags
>    * @nlmsg_seq:   Sequence number
>    * @nlmsg_pid:   Sending process port ID
> - * @nlmsg_data:  Message payload
>    */
>   struct nlmsghdr {
>   	__u32		nlmsg_len;
> @@ -56,7 +55,6 @@ struct nlmsghdr {
>   	__u16		nlmsg_flags;
>   	__u32		nlmsg_seq;
>   	__u32		nlmsg_pid;
> -	__u8		nlmsg_data[];
>   };

This seems to be a sensible change. In general, it's not a good idea
to have variable length objects (flex-array members) in structures used
as headers, and that we know will ultimately be followed by more objects
when embedded inside other structures.

>   
>   /* Flags values */
> @@ -121,6 +119,7 @@ struct nlmsghdr {
>   struct nlmsgerr {
>   	int		error;
>   	struct nlmsghdr msg;
> +	__u8		data[];
>   	/*
>   	 * followed by the message contents unless NETLINK_CAP_ACK was set
>   	 * or the ACK indicates success (error == 0)
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index b8afec32cff6..fe8493d3ae56 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2514,8 +2514,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
>   		if (!nlmsg_append(skb, nlmsg_len(nlh)))
>   			goto err_bad_put;
>   
> -		memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
> -		       nlmsg_len(nlh));
> +		memcpy(errmsg->data, nlmsg_data(nlh), nlmsg_len(nlh));
>   	}
>   
>   	if (tlvlen)
> 
> 

--
Gustavo

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Jakub Kicinski]

On Wed, 16 Nov 2022 18:55:36 -0600 Gustavo A. R. Silva wrote:
> > @@ -56,7 +55,6 @@ struct nlmsghdr {
> >   	__u16		nlmsg_flags;
> >   	__u32		nlmsg_seq;
> >   	__u32		nlmsg_pid;
> > -	__u8		nlmsg_data[];
> >   };  
> 
> This seems to be a sensible change. In general, it's not a good idea
> to have variable length objects (flex-array members) in structures used
> as headers, and that we know will ultimately be followed by more objects
> when embedded inside other structures.

Meaning we should go back to zero-length arrays instead?
Will this not bring back out-of-bound warnings that Kees 
has been fixing?

Is there something in the standard that makes flexible array
at the end of an embedded struct a problem?
Or it's just unlikely compiler people will budge?

AFAICT this is just one of 3 such structs which iproute2 build hits.

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Gustavo A. R. Silva]

On 11/16/22 19:05, Jakub Kicinski wrote:
> On Wed, 16 Nov 2022 18:55:36 -0600 Gustavo A. R. Silva wrote:
>>> @@ -56,7 +55,6 @@ struct nlmsghdr {
>>>    	__u16		nlmsg_flags;
>>>    	__u32		nlmsg_seq;
>>>    	__u32		nlmsg_pid;
>>> -	__u8		nlmsg_data[];
>>>    };
>>
>> This seems to be a sensible change. In general, it's not a good idea
>> to have variable length objects (flex-array members) in structures used
>> as headers, and that we know will ultimately be followed by more objects
>> when embedded inside other structures.
> 
> Meaning we should go back to zero-length arrays instead?

No.
> Is there something in the standard that makes flexible array
> at the end of an embedded struct a problem?

I haven't seen any problems ss long as the flex-array appears last:

struct foo {
	... members
	struct boo {
		... members
		char flex[];
	};
};

struct complex {
	... members
	struct foo embedded;
};

However, the GCC docs[1] mention this:

"A structure containing a flexible array member [..] may not be a
member of a structure [..] (However, these uses are permitted by GCC
as extensions.)"

And in this case it seems that's the reason why GCC doesn't complain?

--
Gustavo

[1] https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Zero-Length.html#Zero-Length

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Jakub Kicinski]

On Wed, 16 Nov 2022 19:20:51 -0600 Gustavo A. R. Silva wrote:
> On 11/16/22 19:05, Jakub Kicinski wrote:
> >> This seems to be a sensible change. In general, it's not a good idea
> >> to have variable length objects (flex-array members) in structures used
> >> as headers, and that we know will ultimately be followed by more objects
> >> when embedded inside other structures.  
> > 
> > Meaning we should go back to zero-length arrays instead?  
> 
> No.

I was asking based on your own commit 1e6e9d0f4859 ("uapi: revert
flexible-array conversions"). This is uAPI as well.

Since we can't prevent user space from wrapping structures seems
like adding a flex member to an existing struct should never be
permitted in uAPI headers? We can just wrap things locally, I guess:

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9ebdf3262015..2af2f8de4043 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2479,7 +2479,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 {
        struct sk_buff *skb;
        struct nlmsghdr *rep;
-       struct nlmsgerr *errmsg;
+       struct hashtag_silly {
+               struct nlmsgerr err;
+               u8 data[];
+       } *errmsg;
        size_t payload = sizeof(*errmsg);
        struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
        unsigned int flags = 0;
@@ -2507,15 +2510,14 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
        if (!rep)
                goto err_bad_put;
        errmsg = nlmsg_data(rep);
-       errmsg->error = err;
-       errmsg->msg = *nlh;
+       errmsg->err.error = err;
+       errmsg->err.msg = *nlh;
 
        if (!(flags & NLM_F_CAPPED)) {
                if (!nlmsg_append(skb, nlmsg_len(nlh)))
                        goto err_bad_put;
 
-               memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
-                      nlmsg_len(nlh));
+               memcpy(errmsg->data, nlmsg_data(nlh), nlmsg_len(nlh));
        }
 
        if (tlvlen)

In this particular case, tho, we're probably better off giving up 
on the flex array and doing nlmsg_data() on both src and dst.

> > Is there something in the standard that makes flexible array
> > at the end of an embedded struct a problem?  
> 
> I haven't seen any problems ss long as the flex-array appears last:
> 
> struct foo {
> 	... members
> 	struct boo {
> 		... members
> 		char flex[];
> 	};
> };
> 
> struct complex {
> 	... members
> 	struct foo embedded;
> };
> 
> However, the GCC docs[1] mention this:
> 
> "A structure containing a flexible array member [..] may not be a
> member of a structure [..] (However, these uses are permitted by GCC
> as extensions.)"
> 
> And in this case it seems that's the reason why GCC doesn't complain?

Seems so, clang's warning is called -Wgnu-variable-sized-type-not-at-end

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Stephen Hemminger]

On Wed, 16 Nov 2022 22:13:06 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 16 Nov 2022 19:20:51 -0600 Gustavo A. R. Silva wrote:
> > On 11/16/22 19:05, Jakub Kicinski wrote:  
> > >> This seems to be a sensible change. In general, it's not a good idea
> > >> to have variable length objects (flex-array members) in structures used
> > >> as headers, and that we know will ultimately be followed by more objects
> > >> when embedded inside other structures.    
> > > 
> > > Meaning we should go back to zero-length arrays instead?    
> > 
> > No.  
> 
> I was asking based on your own commit 1e6e9d0f4859 ("uapi: revert
> flexible-array conversions"). This is uAPI as well.
 
Some of the flex-array conversions fixed build warnings that occur in
iproute2 when using Gcc 12 or later.

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Jakub Kicinski]

On Thu, 17 Nov 2022 08:25:56 -0800 Stephen Hemminger wrote:
> > I was asking based on your own commit 1e6e9d0f4859 ("uapi: revert
> > flexible-array conversions"). This is uAPI as well.  
>  
> Some of the flex-array conversions fixed build warnings that occur in
> iproute2 when using Gcc 12 or later.

Alright, this is getting complicated. I'll post a patch to fix 
the issue I've added and gently place my head back into the sand.

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Kees Cook]

On Thu, Nov 17, 2022 at 12:36:15PM -0800, Jakub Kicinski wrote:
> On Thu, 17 Nov 2022 08:25:56 -0800 Stephen Hemminger wrote:
> > > I was asking based on your own commit 1e6e9d0f4859 ("uapi: revert
> > > flexible-array conversions"). This is uAPI as well.  
> >  
> > Some of the flex-array conversions fixed build warnings that occur in
> > iproute2 when using Gcc 12 or later.
> 
> Alright, this is getting complicated. I'll post a patch to fix 
> the issue I've added and gently place my head back into the sand.

Thanks! I think the path forward is clear. I should not have suggested
adding a flex-array member to the "header" struct lo these many moons
ago. You and Gustavo are right: we need a separate struct with the header
at the beginning, just as iproute2 is doing itself.

As for testing, I can do that if you want -- the goal was to make sure
the final result doesn't trip FORTIFY when built with -fstrict-flex-arrays
(not yet in a released compiler version, but present in both GCC and Clang
truck builds) and with __builtin_dynamic_object_size() enabled (which
is not yet in -next, as it is waiting on the last of ksize() clean-ups).

-- 
Kees Cook

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Jakub Kicinski]

On Thu, 17 Nov 2022 14:35:32 -0800 Kees Cook wrote:
> As for testing, I can do that if you want -- the goal was to make sure
> the final result doesn't trip FORTIFY when built with -fstrict-flex-arrays
> (not yet in a released compiler version, but present in both GCC and Clang
> truck builds) and with __builtin_dynamic_object_size() enabled (which
> is not yet in -next, as it is waiting on the last of ksize() clean-ups).

I got distracted, sorry. Does this work?

-->8--------------

From: Jakub Kicinski <kuba@kernel.org>
Subject: netlink: remove the flex array from struct nlmsghdr

I've added a flex array to struct nlmsghdr to allow accessing
the data easily. But it leads to warnings with clang, when user
space wraps this structure into another struct and the flex
array is not at the end of the container.

Link: https://lore.kernel.org/all/20221114023927.GA685@u2004-local/
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/uapi/linux/netlink.h | 2 --
 net/netlink/af_netlink.c     | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 5da0da59bf01..e2ae82e3f9f7 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -48,7 +48,6 @@ struct sockaddr_nl {
  * @nlmsg_flags: Additional flags
  * @nlmsg_seq:   Sequence number
  * @nlmsg_pid:   Sending process port ID
- * @nlmsg_data:  Message payload
  */
 struct nlmsghdr {
 	__u32		nlmsg_len;
@@ -56,7 +55,6 @@ struct nlmsghdr {
 	__u16		nlmsg_flags;
 	__u32		nlmsg_seq;
 	__u32		nlmsg_pid;
-	__u8		nlmsg_data[];
 };
 
 /* Flags values */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9ebdf3262015..d73091f6bb0f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2514,7 +2514,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		if (!nlmsg_append(skb, nlmsg_len(nlh)))
 			goto err_bad_put;
 
-		memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
+		memcpy(nlmsg_data(&errmsg->msg), nlmsg_data(nlh),
 		       nlmsg_len(nlh));
 	}
 
-- 
2.38.1

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Kees Cook]

On Thu, Nov 17, 2022 at 04:28:22PM -0800, Jakub Kicinski wrote:
> On Thu, 17 Nov 2022 14:35:32 -0800 Kees Cook wrote:
> > As for testing, I can do that if you want -- the goal was to make sure
> > the final result doesn't trip FORTIFY when built with -fstrict-flex-arrays
> > (not yet in a released compiler version, but present in both GCC and Clang
> > truck builds) and with __builtin_dynamic_object_size() enabled (which
> > is not yet in -next, as it is waiting on the last of ksize() clean-ups).
> 
> I got distracted, sorry. Does this work?
> 
> -->8--------------
> 
> From: Jakub Kicinski <kuba@kernel.org>
> Subject: netlink: remove the flex array from struct nlmsghdr
> 
> I've added a flex array to struct nlmsghdr to allow accessing
> the data easily. But it leads to warnings with clang, when user
> space wraps this structure into another struct and the flex
> array is not at the end of the container.
> 
> Link: https://lore.kernel.org/all/20221114023927.GA685@u2004-local/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Thanks! Yes, this seems to be happy with all the future stuff enabled.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  include/uapi/linux/netlink.h | 2 --
>  net/netlink/af_netlink.c     | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 5da0da59bf01..e2ae82e3f9f7 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -48,7 +48,6 @@ struct sockaddr_nl {
>   * @nlmsg_flags: Additional flags
>   * @nlmsg_seq:   Sequence number
>   * @nlmsg_pid:   Sending process port ID
> - * @nlmsg_data:  Message payload
>   */
>  struct nlmsghdr {
>  	__u32		nlmsg_len;
> @@ -56,7 +55,6 @@ struct nlmsghdr {
>  	__u16		nlmsg_flags;
>  	__u32		nlmsg_seq;
>  	__u32		nlmsg_pid;
> -	__u8		nlmsg_data[];
>  };
>  
>  /* Flags values */
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 9ebdf3262015..d73091f6bb0f 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2514,7 +2514,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
>  		if (!nlmsg_append(skb, nlmsg_len(nlh)))
>  			goto err_bad_put;
>  
> -		memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
> +		memcpy(nlmsg_data(&errmsg->msg), nlmsg_data(nlh),
>  		       nlmsg_len(nlh));
>  	}
>  
> -- 
> 2.38.1
> 

-- 
Kees Cook

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[David Ahern]

On 11/17/22 5:28 PM, Jakub Kicinski wrote:
> On Thu, 17 Nov 2022 14:35:32 -0800 Kees Cook wrote:
>> As for testing, I can do that if you want -- the goal was to make sure
>> the final result doesn't trip FORTIFY when built with -fstrict-flex-arrays
>> (not yet in a released compiler version, but present in both GCC and Clang
>> truck builds) and with __builtin_dynamic_object_size() enabled (which
>> is not yet in -next, as it is waiting on the last of ksize() clean-ups).
> 
> I got distracted, sorry. Does this work?
> 
> -->8--------------
> 
> From: Jakub Kicinski <kuba@kernel.org>
> Subject: netlink: remove the flex array from struct nlmsghdr
> 
> I've added a flex array to struct nlmsghdr to allow accessing
> the data easily. But it leads to warnings with clang, when user
> space wraps this structure into another struct and the flex
> array is not at the end of the container.
> 
> Link: https://lore.kernel.org/all/20221114023927.GA685@u2004-local/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/uapi/linux/netlink.h | 2 --
>  net/netlink/af_netlink.c     | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 5da0da59bf01..e2ae82e3f9f7 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -48,7 +48,6 @@ struct sockaddr_nl {
>   * @nlmsg_flags: Additional flags
>   * @nlmsg_seq:   Sequence number
>   * @nlmsg_pid:   Sending process port ID
> - * @nlmsg_data:  Message payload
>   */
>  struct nlmsghdr {
>  	__u32		nlmsg_len;
> @@ -56,7 +55,6 @@ struct nlmsghdr {
>  	__u16		nlmsg_flags;
>  	__u32		nlmsg_seq;
>  	__u32		nlmsg_pid;
> -	__u8		nlmsg_data[];
>  };
>  
>  /* Flags values */
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 9ebdf3262015..d73091f6bb0f 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2514,7 +2514,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
>  		if (!nlmsg_append(skb, nlmsg_len(nlh)))
>  			goto err_bad_put;
>  
> -		memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
> +		memcpy(nlmsg_data(&errmsg->msg), nlmsg_data(nlh),
>  		       nlmsg_len(nlh));
>  	}
>  

LGTM and removing the flex array fixes the iproute2 compile.
Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next v2] netlink: split up copies in the ack construction

[Stephen Hemminger]

On Thu, 17 Nov 2022 12:36:15 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 17 Nov 2022 08:25:56 -0800 Stephen Hemminger wrote:
> > > I was asking based on your own commit 1e6e9d0f4859 ("uapi: revert
> > > flexible-array conversions"). This is uAPI as well.    
> >  
> > Some of the flex-array conversions fixed build warnings that occur in
> > iproute2 when using Gcc 12 or later.  
> 
> Alright, this is getting complicated. I'll post a patch to fix 
> the issue I've added and gently place my head back into the sand.

Building iproute2 with current 6.1-rc5 headers.
Gcc-12 is clean but Clang gets:
    CC       xfrm_state.o
xfrm_state.c:490:8: warning: field 'u' with variable sized type 'union (unnamed union at xfrm_state.c:486:6)' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                                        } u;
                                          ^
This problem started with 6.0.