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

[PATCH net-next RESEND] 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/
Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
RESEND with the right tree in the subject

CC: Florent Fourcot <florent.fourcot@wifirst.fr>
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: 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-next RESEND] genetlink: reject use of nlmsg_flags for new commands

[Pablo Neira Ayuso]

On Thu, Sep 29, 2022 at 07:28:09AM -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.
> 
> Link: https://lore.kernel.org/all/20220928073709.1b93b74a@kernel.org/
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
> Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Reviewed-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> RESEND with the right tree in the subject
> 
> CC: Florent Fourcot <florent.fourcot@wifirst.fr>
> 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: 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;

no bail out for incorrectly set NLM_F_DUMP flag?

> +	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;

While adding new netlink flags is a very rare event, this is going to
make it harder to add new flags to be added in the future, else
userspace has to probe for supported flags first.

Regarding error reporting - even if error reporting in netlink is also
not consistent accross subsystems - I think EINVAL should be used for
malformed netlink messages, eg. a message that is missing a mandatory
attribute.

EOPNOTSUPP might be a better pick?

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

[Jakub Kicinski]

On Thu, 29 Sep 2022 16:53:34 +0200 Pablo Neira Ayuso wrote:
> > +	flags = nlh->nlmsg_flags;
> > +	if ((flags & NLM_F_DUMP) == NLM_F_DUMP) /* DUMP is 2 bits */
> > +		flags &= ~NLM_F_DUMP;  
> 
> no bail out for incorrectly set NLM_F_DUMP flag?

Incorrectly? Special handling is because we want to make sure both bits
are set for DUMP, if they are not we'll not clear them here and the
condition below will fire. Or do you mean some other incorrectness?

> > +	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;  
> 
> While adding new netlink flags is a very rare event, this is going to
> make it harder to add new flags to be added in the future, else
> userspace has to probe for supported flags first.

The only difference in terms of probing is whether the unsupported
case silently ignores the flag or reports a clear error. So I think
I'm only making things better there.

> Regarding error reporting - even if error reporting in netlink is also
> not consistent accross subsystems - I think EINVAL should be used for
> malformed netlink messages, eg. a message that is missing a mandatory
> attribute.
> 
> EOPNOTSUPP might be a better pick?

All current "reserved bits" checking I know of return -EINVAL 
(the strict checks in the ip stack and policy based checks).

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

[Pablo Neira Ayuso]

On Thu, Sep 29, 2022 at 08:06:50AM -0700, Jakub Kicinski wrote:
> On Thu, 29 Sep 2022 16:53:34 +0200 Pablo Neira Ayuso wrote:
> > > +	flags = nlh->nlmsg_flags;
> > > +	if ((flags & NLM_F_DUMP) == NLM_F_DUMP) /* DUMP is 2 bits */
> > > +		flags &= ~NLM_F_DUMP;  
> > 
> > no bail out for incorrectly set NLM_F_DUMP flag?
> 
> Incorrectly? Special handling is because we want to make sure both bits
> are set for DUMP, if they are not we'll not clear them here and the
> condition below will fire. Or do you mean some other incorrectness?

I have seen software in the past setting only one of the bits in the
NLM_F_DUMP bitmask to request a dump. I agree that userspace software
relying in broken semantics and that software should be fixed. What I
am discussing if silently clearing the 2 bits is the best approach.

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

[Jakub Kicinski]

On Thu, 29 Sep 2022 17:49:46 +0200 Pablo Neira Ayuso wrote:
> On Thu, Sep 29, 2022 at 08:06:50AM -0700, Jakub Kicinski wrote:
> > > no bail out for incorrectly set NLM_F_DUMP flag?  
> > 
> > Incorrectly? Special handling is because we want to make sure both bits
> > are set for DUMP, if they are not we'll not clear them here and the
> > condition below will fire. Or do you mean some other incorrectness?  
> 
> I have seen software in the past setting only one of the bits in the
> NLM_F_DUMP bitmask to request a dump. I agree that userspace software
> relying in broken semantics and that software should be fixed. What I
> am discussing if silently clearing the 2 bits is the best approach.

I don't think it is and I don't think I silently clear both.
Here's the code again:

+	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;
+	}

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

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, September 29, 2022 8:55 AM
> To: Pablo Neira Ayuso <pablo@netfilter.org>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; edumazet@google.com;
> pabeni@redhat.com; Johannes Berg <johannes@sipsolutions.net>; Nikolay
> Aleksandrov <razor@blackwall.org>; Nicolas Dichtel
> <nicolas.dichtel@6wind.com>; Guillaume Nault <gnault@redhat.com>; Florent
> Fourcot <florent.fourcot@wifirst.fr>; Florian Westphal <fw@strlen.de>; Jamal
> Hadi Salim <jhs@mojatatu.com>; Keller, Jacob E <jacob.e.keller@intel.com>;
> Hangbin Liu <liuhangbin@gmail.com>
> Subject: Re: [PATCH net-next RESEND] genetlink: reject use of nlmsg_flags for
> new commands
> 
> On Thu, 29 Sep 2022 17:49:46 +0200 Pablo Neira Ayuso wrote:
> > On Thu, Sep 29, 2022 at 08:06:50AM -0700, Jakub Kicinski wrote:
> > > > no bail out for incorrectly set NLM_F_DUMP flag?
> > >
> > > Incorrectly? Special handling is because we want to make sure both bits
> > > are set for DUMP, if they are not we'll not clear them here and the
> > > condition below will fire. Or do you mean some other incorrectness?
> >
> > I have seen software in the past setting only one of the bits in the
> > NLM_F_DUMP bitmask to request a dump. I agree that userspace software
> > relying in broken semantics and that software should be fixed. What I
> > am discussing if silently clearing the 2 bits is the best approach.
> 
> I don't think it is and I don't think I silently clear both.
> Here's the code again:
> 
> +	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;
> +	}

To clarify, my reading of whats going on is that we first see if NLM_F_DUMP is set (both bits!) and if they *are* then we remove them from the set being considered  in the following check, thus they won't be reported as an error.

However, if only one of the bits is set, then they will flag the 2nd error and thus be caught as ambiguous.

Since we modify a local copy of flags, this doesn't modify the actual nlmsg_flags field.

This is somewhat tricky but I don't see another way to approach this unless we duplicate the NL_SET_ERR_MSG.

Thanks,
Jake

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 29 Sep 2022 07:28:09 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next,RESEND] genetlink: reject use of nlmsg_flags for new commands
    https://git.kernel.org/netdev/net-next/c/cff2d762cde6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html