[PATCH net v3] strparser: make sure __strp_recv isn't running before tearing down the parser

[PATCH net v3] strparser: make sure __strp_recv isn't running before tearing down the parser

[Sabrina Dubroca]

The comment above strp_done() claims that if strp is stopped,
strp_recv will no longer be called. That's only true if the caller has
a mechanism (eg locking) to guarantee that strp_recv() calls that
started before strp_stop() have completed by the time we call
strp_done().

This adds a dummy lock_sock/release_sock pair to guarantee that any
in-flight strp_recv() (which runs under either bh_lock_sock or
lock_sock, depending if it's called from ->sk_data_ready or strp_work)
has completed.

Only espintcp can be affected by this race condition, but this patch
makes sure no future user of strp can have the bug.

This could crash on strp->sk ("general mode" of strp), but this mode
has been here for 9 years and has never been used. It'll be gone soon,
no point worrying about it.

Switch the stopped/paused/etc bits to u8's to avoid races between
strp_stop() and strp_pause/unpause().

A reproducer has been published and turns the possible UAF into an
exploit [1].

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Link: https://lore.kernel.org/all/aZLn2Faeg1FB7XOf@v4bel/
Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
Link: https://github.com/m0x41nos/RustyTux [1]
Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
v3: try to make sashiko shut up. drop reviewed-by tag due to code change.
v2: no code change, just add Cc: stable and Reviewed-by: tags
    f06217a9b95b7027286c7df21336e67e334c3e6e.1790674872.git.sd@queasysnail.net
v1: 4c80ea646f95abbd5beca2c081d129fe7d61a0aa.1790344112.git.sd@queasysnail.net

 Documentation/networking/strparser.rst |  2 +-
 include/net/strparser.h                | 10 +++++-----
 net/strparser/strparser.c              |  6 +++++-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/strparser.rst b/Documentation/networking/strparser.rst
index 372106b61e65..d0804c88ad0a 100644
--- a/Documentation/networking/strparser.rst
+++ b/Documentation/networking/strparser.rst
@@ -77,7 +77,7 @@ Functions
 
      strp_done is called to release any resources held by the stream
      parser instance. This must be called after the stream processor
-     has been stopped.
+     has been stopped. The socket lock must not be held.
 
      ::
 
diff --git a/include/net/strparser.h b/include/net/strparser.h
index 0ed73e364faa..881b7ae34498 100644
--- a/include/net/strparser.h
+++ b/include/net/strparser.h
@@ -91,11 +91,11 @@ static inline struct strp_msg *strp_msg(struct sk_buff *skb)
 struct strparser {
 	struct sock *sk;
 
-	u32 stopped : 1;
-	u32 paused : 1;
-	u32 aborted : 1;
-	u32 interrupted : 1;
-	u32 unrecov_intr : 1;
+	u8 stopped;
+	u8 paused;
+	u8 aborted;
+	u8 interrupted;
+	u8 unrecov_intr;
 
 	struct sk_buff **skb_nextp;
 	struct sk_buff *skb_head;
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index a23f4b4dfc67..d5b17d099a0e 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -505,12 +505,16 @@ void strp_unpause(struct strparser *strp)
 EXPORT_SYMBOL_GPL(strp_unpause);
 
 /* strp must already be stopped so that strp_recv will no longer be called.
- * Note that strp_done is not called with the lower socket held.
+ * Note that strp_done must not be called with the lower socket held.
  */
 void strp_done(struct strparser *strp)
 {
 	WARN_ON(!strp->stopped);
 
+	lock_sock(strp->sk);
+	/* sync with pending strp_recv */
+	release_sock(strp->sk);
+
 	cancel_delayed_work_sync(&strp->msg_timer_work);
 	cancel_work_sync(&strp->work);
 
-- 
2.55.0

Re: [PATCH net v3] strparser: make sure __strp_recv isn't running before tearing down the parser

[Hyunwoo Kim]

On Mon, Oct 05, 2026 at 03:50:02PM +0200, Sabrina Dubroca wrote:
> The comment above strp_done() claims that if strp is stopped,
> strp_recv will no longer be called. That's only true if the caller has
> a mechanism (eg locking) to guarantee that strp_recv() calls that
> started before strp_stop() have completed by the time we call
> strp_done().
> 
> This adds a dummy lock_sock/release_sock pair to guarantee that any
> in-flight strp_recv() (which runs under either bh_lock_sock or
> lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> has completed.
> 
> Only espintcp can be affected by this race condition, but this patch
> makes sure no future user of strp can have the bug.
> 
> This could crash on strp->sk ("general mode" of strp), but this mode
> has been here for 9 years and has never been used. It'll be gone soon,
> no point worrying about it.
> 
> Switch the stopped/paused/etc bits to u8's to avoid races between
> strp_stop() and strp_pause/unpause().
> 
> A reproducer has been published and turns the possible UAF into an
> exploit [1].

:(

> 
> Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
> Link: https://lore.kernel.org/all/aZLn2Faeg1FB7XOf@v4bel/
> Link: https://lore.kernel.org/all/aZgpkyTDU3aXe_V0@v4bel/
> Link: https://github.com/m0x41nos/RustyTux [1]
> Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Hyunwoo Kim <imv4bel@gmail.com>

Re: [PATCH net v3] strparser: make sure __strp_recv isn't running before tearing down the parser

[patchwork-bot+netdevbpf]

Hello:

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

On Mon,  5 Oct 2026 15:50:02 +0200 you wrote:
> The comment above strp_done() claims that if strp is stopped,
> strp_recv will no longer be called. That's only true if the caller has
> a mechanism (eg locking) to guarantee that strp_recv() calls that
> started before strp_stop() have completed by the time we call
> strp_done().
> 
> This adds a dummy lock_sock/release_sock pair to guarantee that any
> in-flight strp_recv() (which runs under either bh_lock_sock or
> lock_sock, depending if it's called from ->sk_data_ready or strp_work)
> has completed.
> 
> [...]

Here is the summary with links:
  - [net,v3] strparser: make sure __strp_recv isn't running before tearing down the parser
    https://git.kernel.org/netdev/net/c/0984ebc63179

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

Re: [PATCH net v3] strparser: make sure __strp_recv isn't running before tearing down the parser

[Jakub Kicinski]

On Mon,  5 Oct 2026 15:50:02 +0200 Sabrina Dubroca wrote:
> -	u32 stopped : 1;
> -	u32 paused : 1;
> -	u32 aborted : 1;
> -	u32 interrupted : 1;
> -	u32 unrecov_intr : 1;
> +	u8 stopped;
> +	u8 paused;
> +	u8 aborted;
> +	u8 interrupted;
> +	u8 unrecov_intr;

I gotta say, wish you held off on the cleanups a little longer