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

[netdev-bot+sashiko@kernel.org]

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

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

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 is a regression at this commit only, and a later patch in
  the series fixes it.
- [Low] At this commit, XFRM_MSG_ALLOCSPI still accepts XFRMA_MARK
  without calling verify_mark().

--

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

This commit adds verify_mark(), which rejects an XFRMA_MARK whose value
has bits outside its mask. It is called from verify_newsa_info() and
xfrm_do_migrate_state(). The commit also removes the mark sanitization
from __xfrm_state_insert(), so such marks are rejected instead of being
silently truncated.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index e45aa1ed5b965..602404ffc4d4b 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1732,9 +1732,6 @@ static void __xfrm_state_insert(struct xfrm_state *x)
>  
>  	list_add(&x->km.all, &net->xfrm.state_all);
>  
> -	/* Sanitize mark before store */
> -	x->mark.v &= x->mark.m;
> -

[Severity: Low]
At this commit, can a mark with bits outside its mask still get into the
SAD through XFRM_MSG_ALLOCSPI?

xfrm_alloc_userspi() does not call verify_mark(). xfrm_mark_get() masks
only its return value, and m keeps the raw attribute:

net/xfrm/xfrm_user.c:xfrm_alloc_userspi() {
    ...
	mark = xfrm_mark_get(attrs, &m);
    ...
}

xfrm_find_acq() -> __find_acq_core() then stores the mark unchanged and
adds the larval state to the hash tables directly:

net/xfrm/xfrm_state.c:__find_acq_core() {
    ...
		x->mark.v = m->v;
		x->mark.m = m->m;
    ...
}

So ALLOCSPI with XFRMA_MARK {0x101, 0xff} creates a larval SA that no
masked lookup can match. That part is older than this patch, because
__find_acq_core() never went through __xfrm_state_insert().

The legacy XFRM_MSG_MIGRATE path is newly affected.
xfrm_migrate_copy_old() copies the existing SA's mark into the clone:

net/xfrm/xfrm_policy.c:xfrm_migrate_copy_old() {
    ...
	mp->new_mark               = &x->mark;
}

Before this patch, __xfrm_state_insert() trimmed the clone's mark. With
the sanitization removed, the clone keeps 0x101/0xff.

The next commit in the series, "xfrm: state: reject mark with bits
outside its mask on ALLOCSPI", adds a verify_mark() call to
xfrm_alloc_userspi() before xfrm_mark_get(), which closes this. Could
that change be folded into this patch, or moved ahead of it?

[ ... ]

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a2587c7e796b4..0723d791b8b70 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c

[ ... ]

> @@ -3370,6 +3390,10 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			return err;
>  	}
>  
> +	err = verify_mark(attrs, extack);
> +	if (err)
> +		return err;
> +
>  	copy_from_user_migrate_state(&m, um);
>  
>  	x = xfrm_state_lookup(net, m.old_mark.v & m.old_mark.m,

[Severity: Medium]
verify_mark() checks only attrs[XFRMA_MARK] and returns 0 when that
attribute is absent. Is um->old_mark left unchecked here?

copy_from_user_migrate_state() copies it unchanged:

	ma->old_mark = um->old_mark;

The old SA is looked up with the masked value. So old_mark
{v=0x101, m=0xff} still finds an existing 0x01/0xff SA.

If XFRMA_MARK is not supplied, xfrm_state_clone_and_setup() gives the
clone the raw old_mark:

net/xfrm/xfrm_state.c:xfrm_state_clone_and_setup() {
    ...
	x->mark = m->new_mark ? *m->new_mark : m->old_mark;
    ...
}

Before this patch, __xfrm_state_insert() trimmed that to 0x01/0xff. With
the sanitization removed, the path
xfrm_state_migrate_install() -> xfrm_state_insert() stores the clone as
0x101/0xff.

The masked lookups test (mark & x->mark.m) == x->mark.v, which can never
be true for that mark. Does that leave the original SA deleted and the
migrated SA unreachable?

The later commit "xfrm: state: exact mark/mask match for SPI-keyed
control-plane SA lookups" changes this to
xfrm_state_lookup_exact(net, &m.old_mark, ...). After that change, an
old_mark with bits outside its mask matches no stored SA, and the
request fails with -ESRCH.

That limits the problem to the commits between this one and that one.
Even so, could um->old_mark be checked here too, or could the exact
lookup patch be placed earlier in the series?

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