netdev - Re: [PATCH net-next] docs: netlink: clarify the historical baggage of Netlink flags

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: <20220928073709.1b93b74a@kernel.org>
Date:   Wed, 28 Sep 2022 07:37:09 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Nikolay Aleksandrov <razor@...ckwall.org>
Cc:     nicolas.dichtel@...nd.com,
        Florent Fourcot <florent.fourcot@...irst.fr>,
        netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, Johannes Berg <johannes@...solutions.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jacob Keller <jacob.e.keller@...el.com>,
        Guillaume Nault <gnault@...hat.com>,
        Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH net-next] docs: netlink: clarify the historical baggage
 of Netlink flags

On Wed, 28 Sep 2022 12:21:57 +0300 Nikolay Aleksandrov wrote:
> On 28/09/2022 11:55, Nicolas Dichtel wrote:
> > Le 28/09/2022 à 10:04, Florent Fourcot a écrit :  
> >> About NLM_F_EXCL, I'm not sure that my comment is relevant for your intro.rst
> >> document, but it has another usage in ipset submodule. For IPSET_CMD_DEL,
> >> setting NLM_F_EXCL means "raise an error if entry does not exist before the
> >> delete".  

Interesting.

> > So NLM_F_EXCL could be used with DEL command for netfilter netlink but cannot be
> > used (it overlaps with NLM_F_BULK, see [1]) with DEL command for rtnetlink.
> > Sigh :(
> > 
> > [1] https://lore.kernel.org/netdev/0198618f-7b52-3023-5e9f-b38c49af1677@6wind.com/
> 
> One could argue that's abuse of the api, but since it's part of a different family
> I guess it's ok. NLM_F_EXCL is a modifier of NEW cmd as the comment above it says
> and has never had rtnetlink DEL users.

It's fine in the sense that it works, but it's rather pointless to call
the flags common if they have different semantics depending on the
corner of the kernel they are used in, right?

I was very tempted to send a patch which would validate the top
byte of flags in genetlink for new commands, this way we may some day
find a truly common (as in enforced by the code) use for the bits. 

WDYT?

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 7c136de117eb..0fbaed49e23f 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -739,6 +739,22 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	return err;
 }
 
+static int genl_header_check(struct nlmsghdr *nlh, struct genlmsghdr *hdr)
+{
+	if (hdr->reserved)
+		return -EINVAL;
+
+	/* Flags - lower byte check */
+	if (nlh->nlmsg_flags & 0xff & ~(NLM_F_REQUEST | NLM_F_ACK | NLM_F_ECHO))
+		return -EINVAL;
+	/* Flags - higher byte check */
+	if (nlh->nlmsg_flags & 0xff00 &&
+	    nlh->nlmsg_flags & 0xff00 != NLM_F_DUMP)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int genl_family_rcv_msg(const struct genl_family *family,
 			       struct sk_buff *skb,
 			       struct nlmsghdr *nlh,
@@ -757,7 +773,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
 		return -EINVAL;
 
-	if (hdr->cmd >= family->resv_start_op && hdr->reserved)
+	if (hdr->cmd >= family->resv_start_op && genl_header_check(nlh, hdr))
 		return -EINVAL;
 
 	if (genl_get_cmd(hdr->cmd, family, &op))