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

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: <20221116221306.5a4bd5f8@kernel.org>
Date:   Wed, 16 Nov 2022 22:13:06 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     Kees Cook <keescook@...omium.org>,
        David Ahern <dsahern@...nel.org>, davem@...emloft.net,
        netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH net-next v2] netlink: split up copies in the ack
 construction

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