Re: [PATCH 07/14] xfrm: policy: reject mark with bits outside its mask on add

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch makes XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY
  reject input that mainline currently accepts and that works.

--

xfrm: policy: reject mark with bits outside its mask on add

This patch adds a verify_mark() check to xfrm_add_policy(). The check
rejects XFRMA_MARK values that have bits set outside the mask. It also
removes the mark sanitization from xfrm_policy_insert(), which commit
0b91fda3a1f0 ("xfrm: Sanitize marks before insert") had added.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cc206017d0f50..cea6842e6454d 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1575,9 +1575,6 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  	struct xfrm_policy *delpol;
>  	struct hlist_head *chain;
>  
> -	/* Sanitize mark before store */
> -	policy->mark.v &= policy->mark.m;
> -
>  	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
>  	chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
>  	if (chain)
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 6b53373168b02..c1571c7a23153 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2307,6 +2307,9 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  	err = verify_sec_ctx_len(attrs, extack);
> +	if (err)
> +		return err;
> +	err = verify_mark(attrs, extack);
>  	if (err)
>  		return err;

[Severity: Medium]
Will this reject XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY requests that
current mainline accepts, and that work?

Since 0b91fda3a1f0, a mark such as 0xf42/0xf00 is stored by
xfrm_policy_insert() as 0xf00/0xf00. The policy then matches traffic in
xfrm_policy_match():

net/xfrm/xfrm_policy.c:xfrm_policy_match() {
    ...
	    (fl->flowi_mark & pol->mark.m) != pol->mark.v ||
    ...
}

With this patch, the same request fails in xfrm_add_policy()->verify_mark()
with -EINVAL and "Invalid mark value/mask combination". A userspace IPsec
config that loads and passes traffic on current kernels would no longer
load.

The commit message only says "Reject it instead". It doesn't mention
that this input has worked since 0b91fda3a1f0, or why breaking it in
mainline is acceptable.

The Cc: <stable+noautosel@kernel.org> # avoid breaking existing userspace
ABI trailer suggests this is known to be an ABI change. However, keeping
the patch out of stable doesn't prevent the change in mainline.

Could the lookup failure be fixed without rejecting any input? The GET
and DEL failures seem to come from the exact compare in
xfrm_policy_mark_match():

net/xfrm/xfrm_policy.c:xfrm_policy_mark_match() {
	return mark->v == pol->mark.v && mark->m == pol->mark.m;
}

Applying the same v &= m to the user-supplied lookup mark, for example
in xfrm_mark_get(), would let existing inserts keep working.

Also, the commit message says the policy can't be found "by id". Doesn't
the same compare affect selector lookups through xfrm_policy_bysel_ctx()
as well as xfrm_policy_byid()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com