[PATCH ipsec 0/2] xfrm: fixes for xfrm_state_find under preemption

[PATCH ipsec 0/2] xfrm: fixes for xfrm_state_find under preemption

[Sabrina Dubroca]

While looking at the pcpu_id changes, I found two issues that can
happen if we get preempted and the cpu_id changes. The second patch
takes care of both problems. The first patch also makes sure we don't
use state_ptrs uninitialized, which could currently happen. syzbot
seems to have hit this issue [1].

[1] https://syzkaller.appspot.com/bug?extid=7ed9d47e15e88581dc5b

Sabrina Dubroca (2):
  xfrm: state: initialize state_ptrs earlier in xfrm_state_find
  xfrm: state: use a consistent pcpu_id for xfrm_state_find

 net/xfrm/xfrm_state.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

-- 
2.49.0

[PATCH ipsec 1/2] xfrm: state: initialize state_ptrs earlier in xfrm_state_find

[Sabrina Dubroca]

In case of preemption, xfrm_state_look_at will find a different
pcpu_id and look up states for that other CPU. If we matched a state
for CPU2 in the state_cache while the lookup started on CPU1, we will
jump to "found", but the "best" state that we got will be ignored and
we will enter the "acquire" block. This block uses state_ptrs, which
isn't initialized at this point.

Let's initialize state_ptrs just after taking rcu_read_lock. This will
also prevent a possible misuse in the future, if someone adjusts this
function.

Reported-by: syzbot+7ed9d47e15e88581dc5b@syzkaller.appspotmail.com
Fixes: e952837f3ddb ("xfrm: state: fix out-of-bounds read during lookup")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 07fe8e5daa32..ff6813ecc6df 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1389,6 +1389,8 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	sequence = read_seqcount_begin(&net->xfrm.xfrm_state_hash_generation);
 
 	rcu_read_lock();
+	xfrm_hash_ptrs_get(net, &state_ptrs);
+
 	hlist_for_each_entry_rcu(x, &pol->state_cache_list, state_cache) {
 		if (x->props.family == encap_family &&
 		    x->props.reqid == tmpl->reqid &&
@@ -1429,8 +1431,6 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	else if (acquire_in_progress) /* XXX: acquire_in_progress should not happen */
 		WARN_ON(1);
 
-	xfrm_hash_ptrs_get(net, &state_ptrs);
-
 	h = __xfrm_dst_hash(daddr, saddr, tmpl->reqid, encap_family, state_ptrs.hmask);
 	hlist_for_each_entry_rcu(x, state_ptrs.bydst + h, bydst) {
 #ifdef CONFIG_XFRM_OFFLOAD
-- 
2.49.0

Re: [PATCH ipsec 1/2] xfrm: state: initialize state_ptrs earlier in xfrm_state_find

[Florian Westphal]

Sabrina Dubroca <sd@queasysnail.net> wrote:
> In case of preemption, xfrm_state_look_at will find a different
> pcpu_id and look up states for that other CPU. If we matched a state
> for CPU2 in the state_cache while the lookup started on CPU1, we will
> jump to "found", but the "best" state that we got will be ignored and
> we will enter the "acquire" block. This block uses state_ptrs, which
> isn't initialized at this point.

Yep, I missed the "goto" and cc doesn't complain either.

> Let's initialize state_ptrs just after taking rcu_read_lock. This will
> also prevent a possible misuse in the future, if someone adjusts this
> function.

Thanks for fixing this bug.

Reviewed-by: Florian Westphal <fw@strlen.de>

[PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state_find

[Sabrina Dubroca]

If we get preempted during xfrm_state_find, we could run
xfrm_state_look_at using a different pcpu_id than the one
xfrm_state_find saw. This could lead to ignoring states that should
have matched, and triggering acquires on a CPU that already has a pcpu
state.

    xfrm_state_find starts on CPU1
    pcpu_id = 1
    lookup starts
    <preemption, we're now on CPU2>
    xfrm_state_look_at pcpu_id = 2
       finds a state
found:
    best->pcpu_num != pcpu_id (2 != 1)
    if (!x && !error && !acquire_in_progress) {
        ...
        xfrm_state_alloc
        xfrm_init_tempstate
        ...

This can be avoided by passing the original pcpu_id down to all
xfrm_state_look_at() calls.

Also switch to raw_smp_processor_id, disabling preempting just to
re-enable it immediately doesn't really make sense.

Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 net/xfrm/xfrm_state.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index ff6813ecc6df..3dc78ef2bf7d 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1307,14 +1307,8 @@ static void xfrm_hash_grow_check(struct net *net, int have_hash_collision)
 static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
 			       const struct flowi *fl, unsigned short family,
 			       struct xfrm_state **best, int *acq_in_progress,
-			       int *error)
+			       int *error, unsigned int pcpu_id)
 {
-	/* We need the cpu id just as a lookup key,
-	 * we don't require it to be stable.
-	 */
-	unsigned int pcpu_id = get_cpu();
-	put_cpu();
-
 	/* Resolution logic:
 	 * 1. There is a valid state with matching selector. Done.
 	 * 2. Valid state with inappropriate selector. Skip.
@@ -1381,8 +1375,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 	/* We need the cpu id just as a lookup key,
 	 * we don't require it to be stable.
 	 */
-	pcpu_id = get_cpu();
-	put_cpu();
+	pcpu_id = raw_smp_processor_id();
 
 	to_put = NULL;
 
@@ -1402,7 +1395,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
 			xfrm_state_look_at(pol, x, fl, encap_family,
-					   &best, &acquire_in_progress, &error);
+					   &best, &acquire_in_progress, &error, pcpu_id);
 	}
 
 	if (best)
@@ -1419,7 +1412,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
 			xfrm_state_look_at(pol, x, fl, family,
-					   &best, &acquire_in_progress, &error);
+					   &best, &acquire_in_progress, &error, pcpu_id);
 	}
 
 cached:
@@ -1460,7 +1453,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
 			xfrm_state_look_at(pol, x, fl, family,
-					   &best, &acquire_in_progress, &error);
+					   &best, &acquire_in_progress, &error, pcpu_id);
 	}
 	if (best || acquire_in_progress)
 		goto found;
@@ -1495,7 +1488,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
 		    tmpl->id.proto == x->id.proto &&
 		    (tmpl->id.spi == x->id.spi || !tmpl->id.spi))
 			xfrm_state_look_at(pol, x, fl, family,
-					   &best, &acquire_in_progress, &error);
+					   &best, &acquire_in_progress, &error, pcpu_id);
 	}
 
 found:
-- 
2.49.0

Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state_find

[Florian Westphal]

Sabrina Dubroca <sd@queasysnail.net> wrote:
> If we get preempted during xfrm_state_find, we could run
> xfrm_state_look_at using a different pcpu_id than the one
> xfrm_state_find saw.

[..]

> This can be avoided by passing the original pcpu_id down to all
> xfrm_state_look_at() calls.

FWIW I found this get/put pair slightly confusing as well, so:

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state_find

[Steffen Klassert]

On Fri, May 23, 2025 at 05:11:18PM +0200, Sabrina Dubroca wrote:
> If we get preempted during xfrm_state_find, we could run
> xfrm_state_look_at using a different pcpu_id than the one
> xfrm_state_find saw. This could lead to ignoring states that should
> have matched, and triggering acquires on a CPU that already has a pcpu
> state.
> 
>     xfrm_state_find starts on CPU1
>     pcpu_id = 1
>     lookup starts
>     <preemption, we're now on CPU2>
>     xfrm_state_look_at pcpu_id = 2
>        finds a state
> found:
>     best->pcpu_num != pcpu_id (2 != 1)
>     if (!x && !error && !acquire_in_progress) {
>         ...
>         xfrm_state_alloc
>         xfrm_init_tempstate
>         ...
> 
> This can be avoided by passing the original pcpu_id down to all
> xfrm_state_look_at() calls.
> 
> Also switch to raw_smp_processor_id, disabling preempting just to
> re-enable it immediately doesn't really make sense.
> 
> Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  net/xfrm/xfrm_state.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index ff6813ecc6df..3dc78ef2bf7d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1307,14 +1307,8 @@ static void xfrm_hash_grow_check(struct net *net, int have_hash_collision)
>  static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
>  			       const struct flowi *fl, unsigned short family,
>  			       struct xfrm_state **best, int *acq_in_progress,
> -			       int *error)
> +			       int *error, unsigned int pcpu_id)
>  {
> -	/* We need the cpu id just as a lookup key,
> -	 * we don't require it to be stable.
> -	 */
> -	unsigned int pcpu_id = get_cpu();
> -	put_cpu();
> -
>  	/* Resolution logic:
>  	 * 1. There is a valid state with matching selector. Done.
>  	 * 2. Valid state with inappropriate selector. Skip.
> @@ -1381,8 +1375,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	/* We need the cpu id just as a lookup key,
>  	 * we don't require it to be stable.
>  	 */
> -	pcpu_id = get_cpu();
> -	put_cpu();
> +	pcpu_id = raw_smp_processor_id();

This codepath can be taken from the forwarding path with preemtion
disabled. raw_smp_processor_id will trigger a warning in that case,
maybe better to use raw_cpu_ptr?

Thanks!

Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state_find

[Florian Westphal]

Steffen Klassert <steffen.klassert@secunet.com> wrote:
> > -	pcpu_id = get_cpu();
> > -	put_cpu();
> > +	pcpu_id = raw_smp_processor_id();
> 
> This codepath can be taken from the forwarding path with preemtion
> disabled. raw_smp_processor_id will trigger a warning in that case,

Are you sure? smp_processor_id() emits a warning when called from
preemptible context, raw_smp_processor_id() should not do that.

We use raw_smp_processor_id from various netfilter modules as well
and I never saw preemption warnings.

Re: [PATCH ipsec 2/2] xfrm: state: use a consistent pcpu_id in xfrm_state_find

[Steffen Klassert]

On Mon, May 26, 2025 at 03:57:04PM +0200, Florian Westphal wrote:
> Steffen Klassert <steffen.klassert@secunet.com> wrote:
> > > -	pcpu_id = get_cpu();
> > > -	put_cpu();
> > > +	pcpu_id = raw_smp_processor_id();
> > 
> > This codepath can be taken from the forwarding path with preemtion
> > disabled. raw_smp_processor_id will trigger a warning in that case,
> 
> Are you sure? smp_processor_id() emits a warning when called from
> preemptible context, raw_smp_processor_id() should not do that.
> 
> We use raw_smp_processor_id from various netfilter modules as well
> and I never saw preemption warnings.

You are right, I just saw smp_processor_id. Sorry for the noise!

Re: [PATCH ipsec 0/2] xfrm: fixes for xfrm_state_find under preemption

[Steffen Klassert]

On Fri, May 23, 2025 at 05:11:16PM +0200, Sabrina Dubroca wrote:
> While looking at the pcpu_id changes, I found two issues that can
> happen if we get preempted and the cpu_id changes. The second patch
> takes care of both problems. The first patch also makes sure we don't
> use state_ptrs uninitialized, which could currently happen. syzbot
> seems to have hit this issue [1].
> 
> [1] https://syzkaller.appspot.com/bug?extid=7ed9d47e15e88581dc5b
> 
> Sabrina Dubroca (2):
>   xfrm: state: initialize state_ptrs earlier in xfrm_state_find
>   xfrm: state: use a consistent pcpu_id for xfrm_state_find

Applied, thanks a lot!