[PATCH] 9p: pin request under client lock in p9_tag_lookup

[PATCH] 9p: pin request under client lock in p9_tag_lookup

[Jérémy Jean]

9P clients keep separate request tables but share one request cache.
p9_tag_lookup() can race with the final p9_req_put() and pin a request
reused by another client. Both clients can then complete it, freeing
its response while the caller still parses it, creating a UAF that
KASAN reports as:

  BUG: KASAN: slab-use-after-free in p9pdu_readf+0x362/0x2130
  Read of size 4 at addr ffff888009520000 by task repro/82

  Freed by task 50:
   p9_req_put+0x1a4/0x1f0
   p9_read_work+0x916/0xf80

Hold the client lock while finding and pinning the request, preventing
removal and reuse between these operations.

Fixes: 728356dedeff ("9p: Add refcount to p9_req_t")
Cc: stable@kernel.org
Assisted-by: LLM
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/9p/client.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index ef64546c6d52..fd72f05128d1 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -235,24 +235,18 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint t_size, uint r_size,
 struct p9_req_t *p9_tag_lookup(struct p9_client *c, u16 tag)
 {
 	struct p9_req_t *req;
+	unsigned long flags;
 
-	rcu_read_lock();
-again:
+	/*
+	 * Pin the request while removal from this client's IDR is excluded.
+	 * Tag equality alone cannot distinguish a request recycled for another
+	 * client from the same SLAB_TYPESAFE_BY_RCU cache.
+	 */
+	spin_lock_irqsave(&c->lock, flags);
 	req = idr_find(&c->reqs, tag);
-	if (req) {
-		/* We have to be careful with the req found under rcu_read_lock
-		 * Thanks to SLAB_TYPESAFE_BY_RCU we can safely try to get the
-		 * ref again without corrupting other data, then check again
-		 * that the tag matches once we have the ref
-		 */
-		if (!p9_req_try_get(req))
-			goto again;
-		if (req->tc.tag != tag) {
-			p9_req_put(c, req);
-			goto again;
-		}
-	}
-	rcu_read_unlock();
+	if (req && !p9_req_try_get(req))
+		req = NULL;
+	spin_unlock_irqrestore(&c->lock, flags);
 
 	return req;
 }
-- 
2.47.3

Re: [PATCH] 9p: pin request under client lock in p9_tag_lookup

[Dominique Martinet]

Jérémy Jean wrote on Wed, Sep 30, 2026 at 08:13:58PM +0000:
> 9P clients keep separate request tables but share one request cache.
> p9_tag_lookup() can race with the final p9_req_put() and pin a request

Ah.. Right, just checking the tag isn't enough, we also need to check
the client... I'm not sure why I had assumed the caches were not merged,
they're only not merged with different types..

So technically we could add a reference or some kind of id for the
client and check that, but given SLAB_TYPESAFE_BY_RCU is hard to grok I
guess it's much simpler to just take the lock and be done with it...
(in that case we can get rid of SLAB_TYPESAFE_BY_RCU altogether, the
lock is enough guarantee that the memory is valid until we get the ref
here)

It's far from a new bug so I'd rather not rush this just to go back to
rcu a few weeks later, give me a bit of time to check how much impact
taking the lock here adds so we can decide. There's enough leeway in the
p9_req_t struct that we can check the client for free if we care...

Meanwhile, if someone else has an opinon feel free to chip in
-- 
Dominique Martinet | Asmadeus

Re: [PATCH] 9p: pin request under client lock in p9_tag_lookup

[Jérémy Jean]

On 2026-10-29 06:50, Dominique Martinet wrote:
> Jérémy Jean wrote on Wed, Sep 30, 2026 at 08:13:58PM +0000:
>> 9P clients keep separate request tables but share one request cache.
>> p9_tag_lookup() can race with the final p9_req_put() and pin a request
> 
> Ah.. Right, just checking the tag isn't enough, we also need to check
> the client... I'm not sure why I had assumed the caches were not 
> merged,
> they're only not merged with different types..
> 
> So technically we could add a reference or some kind of id for the
> client and check that, but given SLAB_TYPESAFE_BY_RCU is hard to grok I
> guess it's much simpler to just take the lock and be done with it...
> (in that case we can get rid of SLAB_TYPESAFE_BY_RCU altogether, the
> lock is enough guarantee that the memory is valid until we get the ref
> here)
> 
> It's far from a new bug so I'd rather not rush this just to go back to
> rcu a few weeks later, give me a bit of time to check how much impact
> taking the lock here adds so we can decide. There's enough leeway in 
> the
> p9_req_t struct that we can check the client for free if we care...
> 
> Meanwhile, if someone else has an opinon feel free to chip in

Hello Dominique,

Thanks for the reply, and I'm happy to discuss the best way to fix
the UAF.

I am not so familiar with that code base and I have a hard time
deciding the actual impact of the lock. As you said, it might be
possible to keep p9_tag_lookup() lockless by storing the owning
client in each request and then validating both the client and the
tag after successfully incrementing the refcount.

The two helpers refcount_inc_not_zero_acquire() and
refcount_set_release() should ensure that the client and tag checks
see the newly initialized request state after reuse. Then, the
mismatched reference could be dropped through req->client. This
should stop a stale lookup from mistaking a recycled request from
another client for the original one.

Below is a tentative patch that applies this strategy. I have
verified with my reproducer that it indeed fixed the UAF. What
do you think of it? Let me know if you'd want me to send a v2.

Best regards,
Jérémy

---

diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 55c6cb54bd254..a46187833cb8d 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -84,6 +84,7 @@ struct p9_req_t {
         int status;
         int t_err;
         refcount_t refcount;
+       struct p9_client *client;
         wait_queue_head_t wq;
         struct p9_fcall tc;
         struct p9_fcall rc;
@@ -333,7 +334,7 @@ static inline void p9_req_get(struct p9_req_t *r)

  static inline int p9_req_try_get(struct p9_req_t *r)
  {
-       return refcount_inc_not_zero(&r->refcount);
+       return refcount_inc_not_zero_acquire(&r->refcount);
  }

  int p9_req_put(struct p9_client *c, struct p9_req_t *r);
diff --git a/net/9p/client.c b/net/9p/client.c
index ef64546c6d52a..31e4bfa4901e3 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -181,9 +181,10 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint 
t_size, uint r_size,
         p9pdu_reset(&req->rc);
         req->t_err = 0;
         req->status = REQ_STATUS_ALLOC;
+       req->client = c;
         /* refcount needs to be set to 0 before inserting into the idr
          * so p9_tag_lookup does not accept a request that is not fully
-        * initialized. refcount_set to 2 below will mark request ready.
+        * initialized. refcount_set_release to 2 below will mark 
request ready.
          */
         refcount_set(&req->refcount, 0);
         init_waitqueue_head(&req->wq);
@@ -212,7 +213,7 @@ p9_tag_alloc(struct p9_client *c, int8_t type, uint 
t_size, uint r_size,
          * callback), so p9_client_cb eats the second ref there
          * as the pointer is duplicated directly by virtqueue_add_sgs()
          */
-       refcount_set(&req->refcount, 2);
+       refcount_set_release(&req->refcount, 2);

         return req;

@@ -243,12 +244,12 @@ struct p9_req_t *p9_tag_lookup(struct p9_client 
*c, u16 tag)
                 /* We have to be careful with the req found under 
rcu_read_lock
                  * Thanks to SLAB_TYPESAFE_BY_RCU we can safely try to 
get the
                  * ref again without corrupting other data, then check 
again
-                * that the tag matches once we have the ref
+                * that the client and tag match once we have the ref
                  */
                 if (!p9_req_try_get(req))
                         goto again;
-               if (req->tc.tag != tag) {
-                       p9_req_put(c, req);
+               if (req->client != c || req->tc.tag != tag) {
+                       p9_req_put(req->client, req);
                         goto again;
                 }
         }

Re: [PATCH] 9p: pin request under client lock in p9_tag_lookup

[Dominique Martinet]

Jérémy Jean wrote on Thu, Oct 29, 2026 at 11:56:03AM +0100:
> I am not so familiar with that code base and I have a hard time
> deciding the actual impact of the lock.

Each request is already taking the lock twice (when allocating/freeing
the tag in the idr), so I'm honestly not sure how much adding a third
locking call will change in practice without testing (and if I wanted to
be pedantic I'd say I can't even test this properly as I don't have any
numa hardware that'd be impacted more strongly by this)...

One of my worries was that some transports call this lookup function in
a irq (e.g. rdma depending on hardware), but that's another thing I
can't test /shrug

> As you said, it might be possible to keep p9_tag_lookup() lockless by
> storing the owning client in each request and then validating both the
> client and the tag after successfully incrementing the refcount.
> 
> The two helpers refcount_inc_not_zero_acquire() and
> refcount_set_release() should ensure that the client and tag checks
> see the newly initialized request state after reuse. Then, the
> mismatched reference could be dropped through req->client. This
> should stop a stale lookup from mistaking a recycled request from
> another client for the original one.
> 
> Below is a tentative patch that applies this strategy. I have
> verified with my reproducer that it indeed fixed the UAF. What
> do you think of it? Let me know if you'd want me to send a v2.

That's pretty much what I had in mind, yes.
I wasn't thinking of the _aquire/_release refcount variants but after
checking I agree they're needed, so I'm tempted to say it's another
argument in favor of stopping SLAB_TYPESAFE_BY_RCU...!

On the opposite side if we keep your current v1 then I'd get rid of all
references to SLAB_TYPESAFE_BY_RCU - it's no longer needed with the
lock, and the allocation no longer needs to split refcount init and set
either.

Anyway, please give me a bit of time to run some quick benchmark with
and without lock even if it likely won't mean much, and we can iterate
from there; I don't think this warrants a v2 patch at this point.

(It will be at best a few days, I'm not expecting to take this in for
7.4)

Thanks,
-- 
Dominique Martinet | Asmadeus