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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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?