[PATCH net] genetlink: reject use of nlmsg_flags for new commands

[PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Jakub Kicinski]

Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
introduced extra validation for genetlink headers. We had to gate it
to only apply to new commands, to maintain bug-wards compatibility.
Use this opportunity (before the new checks make it to Linus's tree)
to add more conditions.

Validate that Generic Netlink families do not use nlmsg_flags outside
of the well-understood set.

Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: Florent Fourcot <florent.fourcot@wifirst.fr>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
CC: Johannes Berg <johannes@sipsolutions.net>
CC: Pablo Neira Ayuso <pablo@netfilter.org>
CC: Florian Westphal <fw@strlen.de>
CC: Jamal Hadi Salim <jhs@mojatatu.com>
CC: Jacob Keller <jacob.e.keller@intel.com>
CC: Guillaume Nault <gnault@redhat.com>
CC: Hangbin Liu <liuhangbin@gmail.com>
---
 net/netlink/genetlink.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 7c136de117eb..39b7c00e4cef 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -739,6 +739,36 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	return err;
 }
 
+static int genl_header_check(const struct genl_family *family,
+			     struct nlmsghdr *nlh, struct genlmsghdr *hdr,
+			     struct netlink_ext_ack *extack)
+{
+	u16 flags;
+
+	/* Only for commands added after we started validating */
+	if (hdr->cmd < family->resv_start_op)
+		return 0;
+
+	if (hdr->reserved) {
+		NL_SET_ERR_MSG(extack, "genlmsghdr.reserved field is not 0");
+		return -EINVAL;
+	}
+
+	/* Old netlink flags have pretty loose semantics, allow only the flags
+	 * consumed by the core where we can enforce the meaning.
+	 */
+	flags = nlh->nlmsg_flags;
+	if ((flags & NLM_F_DUMP) == NLM_F_DUMP) /* DUMP is 2 bits */
+		flags &= ~NLM_F_DUMP;
+	if (flags & ~(NLM_F_REQUEST | NLM_F_ACK | NLM_F_ECHO)) {
+		NL_SET_ERR_MSG(extack,
+			       "ambiguous or reserved bits set in nlmsg_flags");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int genl_family_rcv_msg(const struct genl_family *family,
 			       struct sk_buff *skb,
 			       struct nlmsghdr *nlh,
@@ -757,7 +787,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 (genl_header_check(family, nlh, hdr, extack))
 		return -EINVAL;
 
 	if (genl_get_cmd(hdr->cmd, family, &op))
-- 
2.37.3

Re: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Johannes Berg]

On Wed, 2022-09-28 at 11:35 -0700, Jakub Kicinski wrote:
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.

Makes sense.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Thanks for doing this :)

johannes

Re: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Nikolay Aleksandrov]

On 28/09/2022 21:35, Jakub Kicinski wrote:
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.
> 
> Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> CC: Florent Fourcot <florent.fourcot@wifirst.fr>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Pablo Neira Ayuso <pablo@netfilter.org>
> CC: Florian Westphal <fw@strlen.de>
> CC: Jamal Hadi Salim <jhs@mojatatu.com>
> CC: Jacob Keller <jacob.e.keller@intel.com>
> CC: Guillaume Nault <gnault@redhat.com>
> CC: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/netlink/genetlink.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 

Thanks!
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Nicolas Dichtel]

Le 28/09/2022 à 20:35, Jakub Kicinski a écrit :
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.
> 
> Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> CC: Florent Fourcot <florent.fourcot@wifirst.fr>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Pablo Neira Ayuso <pablo@netfilter.org>
> CC: Florian Westphal <fw@strlen.de>
> CC: Jamal Hadi Salim <jhs@mojatatu.com>
> CC: Jacob Keller <jacob.e.keller@intel.com>
> CC: Guillaume Nault <gnault@redhat.com>
> CC: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Thanks!

Re: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Jakub Kicinski]

On Wed, 28 Sep 2022 11:35:15 -0700 Jakub Kicinski wrote:
>  [PATCH net] genetlink: reject use of nlmsg_flags for new commands

I managed to mis-type the subject, should have been net-next,
obviously. I'll resend tomorrow.

Re: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Guillaume Nault]

On Wed, Sep 28, 2022 at 11:35:15AM -0700, Jakub Kicinski wrote:
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.

Reviewed-by: Guillaume Nault <gnault@redhat.com>

RE: [PATCH net] genetlink: reject use of nlmsg_flags for new commands

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, September 28, 2022 11:35 AM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; edumazet@google.com; pabeni@redhat.com; Jakub
> Kicinski <kuba@kernel.org>; Florent Fourcot <florent.fourcot@wifirst.fr>; Nikolay
> Aleksandrov <razor@blackwall.org>; Nicolas Dichtel
> <nicolas.dichtel@6wind.com>; Johannes Berg <johannes@sipsolutions.net>;
> Pablo Neira Ayuso <pablo@netfilter.org>; Florian Westphal <fw@strlen.de>;
> Jamal Hadi Salim <jhs@mojatatu.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>; Guillaume Nault <gnault@redhat.com>; Hangbin Liu
> <liuhangbin@gmail.com>
> Subject: [PATCH net] genetlink: reject use of nlmsg_flags for new commands
> 
> Commit 9c5d03d36251 ("genetlink: start to validate reserved header bytes")
> introduced extra validation for genetlink headers. We had to gate it
> to only apply to new commands, to maintain bug-wards compatibility.
> Use this opportunity (before the new checks make it to Linus's tree)
> to add more conditions.
> 
> Validate that Generic Netlink families do not use nlmsg_flags outside
> of the well-understood set.
> 
> Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> CC: Florent Fourcot <florent.fourcot@wifirst.fr>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> CC: Johannes Berg <johannes@sipsolutions.net>
> CC: Pablo Neira Ayuso <pablo@netfilter.org>
> CC: Florian Westphal <fw@strlen.de>
> CC: Jamal Hadi Salim <jhs@mojatatu.com>
> CC: Jacob Keller <jacob.e.keller@intel.com>
> CC: Guillaume Nault <gnault@redhat.com>
> CC: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/netlink/genetlink.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 7c136de117eb..39b7c00e4cef 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -739,6 +739,36 @@ static int genl_family_rcv_msg_doit(const struct
> genl_family *family,
>  	return err;
>  }
> 
> +static int genl_header_check(const struct genl_family *family,
> +			     struct nlmsghdr *nlh, struct genlmsghdr *hdr,
> +			     struct netlink_ext_ack *extack)
> +{
> +	u16 flags;
> +
> +	/* Only for commands added after we started validating */
> +	if (hdr->cmd < family->resv_start_op)
> +		return 0;
> +
> +	if (hdr->reserved) {
> +		NL_SET_ERR_MSG(extack, "genlmsghdr.reserved field is not 0");
> +		return -EINVAL;
> +	}
> +
> +	/* Old netlink flags have pretty loose semantics, allow only the flags
> +	 * consumed by the core where we can enforce the meaning.
> +	 */
> +	flags = nlh->nlmsg_flags;
> +	if ((flags & NLM_F_DUMP) == NLM_F_DUMP) /* DUMP is 2 bits */
> +		flags &= ~NLM_F_DUMP;

Ok so this check is making sure that flags has BOTH NLM_F_ROOT and NLM_F_MATCH or neither, but not only one of them?

That's why it can't go in the following section, because we want to allow NLM_F_DUMP but not NLM_F_ROOT or NLM_F_MATCH on its own.

Ok.

> +	if (flags & ~(NLM_F_REQUEST | NLM_F_ACK | NLM_F_ECHO)) {
> +		NL_SET_ERR_MSG(extack,
> +			       "ambiguous or reserved bits set in nlmsg_flags");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int genl_family_rcv_msg(const struct genl_family *family,
>  			       struct sk_buff *skb,
>  			       struct nlmsghdr *nlh,
> @@ -757,7 +787,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 (genl_header_check(family, nlh, hdr, extack))
>  		return -EINVAL;
> 
>  	if (genl_get_cmd(hdr->cmd, family, &op))
> --
> 2.37.3

Looks good to me!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>