Re: [PATCH] net/9p: Fix uaf / refcnt underflow for req object in virtio
From: David Vernet
Date: Mon Jul 22 2024 - 15:50:11 EST
On Mon, Jul 22, 2024 at 03:59:21PM +0900, asmadeus@xxxxxxxxxxxxx wrote:
> David Vernet wrote on Tue, Jul 09, 2024 at 11:29:04AM -0500:
> > We can currently encounter an underflow when freeing an rpc request object
> > when using a virtio 9p transport, as shown by stress-ng:
> >
> > $ stress-ng --exec 1 -t 1
> > ...
> > [ 5.188359] ------------[ cut here ]------------
> > [ 5.211094] refcount_t: underflow; use-after-free.
> > [ 5.230781] WARNING: CPU: 6 PID: 557 at lib/refcount.c:28 refcount_warn_saturate+0xb4/0x140
> > [ 5.247551] Modules linked in:
> > [ 5.258476] CPU: 6 PID: 557 Comm: stress-ng-exec- Not tainted 6.10.0-rc2-virtme #26
> > [ 5.276072] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014
> > [ 5.294310] RIP: 0010:refcount_warn_saturate+0xb4/0x140
> > [ 5.313818] Code: ff 90 0f 0b 90 90 e9 16 07 91 00 cc 80 3d f9 14 83 01 00 75 90 c6 05 f0 14 83 01 01 90 48 c7 c7 97 ad 48 84 e8 5d d2 b5 ff 90 <0f> 0b 90 90 e9 ee 06 91 00 cc 80 3d ce 14 83 01 00 0f 85 64 ff ff
> > [ 5.342462] RSP: 0018:ffffa9ac80a47a28 EFLAGS: 00010246
> > [ 5.345307] RAX: b2786dec9d38b800 RBX: ffff9b67e704eaa8 RCX: ffffffff84c6e950
> > [ 5.361743] RDX: 0000000000000002 RSI: 0000000100005ceb RDI: ffffffff84c9ed60
> > [ 5.384172] RBP: 0000000000000000 R08: 0000000000001ceb R09: ffffffff84c6ed60
> > [ 5.408242] R10: 00000000000056c1 R11: 0000000000000003 R12: 00000000fffffe00
> > [ 5.425085] R13: ffffffff8452db57 R14: ffff9b67cb3b2f00 R15: ffff9b67c4e33ec0
> > [ 5.444305] FS: 00007fb7ba9e16c0(0000) GS:ffff9b67fe780000(0000) knlGS:0000000000000000
> > [ 5.464612] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 5.481423] CR2: 00007fffcdf76fe8 CR3: 000000000c142000 CR4: 0000000000750ef0
> > [ 5.498275] PKRU: 55555554
> > [ 5.502138] Call Trace:
> > [ 5.505372] <TASK>
> > [ 5.507353] ? __warn+0xc9/0x1c0
> > [ 5.511056] ? refcount_warn_saturate+0xb4/0x140
> > [ 5.520082] ? report_bug+0x148/0x1e0
> > [ 5.531777] ? handle_bug+0x3e/0x70
> > [ 5.543371] ? exc_invalid_op+0x1a/0x50
> > [ 5.557119] ? asm_exc_invalid_op+0x1a/0x20
> > [ 5.564184] ? refcount_warn_saturate+0xb4/0x140
> > [ 5.576789] ? refcount_warn_saturate+0xb3/0x140
> > [ 5.589334] p9_req_put+0x9e/0xf0
> > [ 5.598119] p9_client_zc_rpc+0x3ac/0x460
> > [ 5.611438] ? wake_page_function+0x41/0xa0
> > [ 5.620745] ? __wake_up_locked_key+0x3f/0x70
> > [ 5.631290] p9_client_read_once+0xd8/0x2d0
> > [ 5.644251] p9_client_read+0x3e/0x70
> > [ 5.655285] v9fs_issue_read+0x47/0x90
> > [ 5.668043] netfs_begin_read+0x3f2/0x880
> > [ 5.673140] ? srso_alias_return_thunk+0x5/0xfbef5
> > [ 5.676800] ? netfs_alloc_request+0x21a/0x2b0
> > [ 5.679999] netfs_read_folio+0xde/0x380
> > [ 5.685441] ? srso_alias_return_thunk+0x5/0xfbef5
> > [ 5.695729] ? __filemap_get_folio+0x158/0x2a0
> > [ 5.713327] filemap_read_folio+0x1f/0x70
> > [ 5.726596] filemap_fault+0x28d/0x510
> > [ 5.737644] __do_fault+0x42/0xb0
> > [ 5.747996] handle_mm_fault+0x59c/0x13a0
> > [ 5.759156] ? srso_alias_return_thunk+0x5/0xfbef5
> > [ 5.776631] ? mt_find+0x2da/0x400
> > [ 5.785758] do_user_addr_fault+0x1fd/0x790
> > [ 5.800640] ? srso_alias_return_thunk+0x5/0xfbef5
> > [ 5.813852] exc_page_fault+0x68/0x180
> > [ 5.822403] asm_exc_page_fault+0x26/0x30
> > [ 5.933431] </TASK>
> > [ 5.941515] ---[ end trace 0000000000000000 ]---
> >
> > The sequence that causes this underflow is as follows:
> >
> > 1. The virtio transport returns -ERESTARTSYS from p9_virtio_zc_request()
> > due to virtqueue_add_sgs() returning -ENOSPC, and wait_event_killable()
> > eventually returning -ERESTARTSYS.
> >
> > 2. The request is never kicked, so its reference is dropped at the end of
> > p9_virtio_zc_request(). This is the first of the two available
> > references.
> >
> > 3. In p9_client_zc_rpc(), the client is still connected, so we call
> > p9_client_flush() which drops the second and final reference in
> > the callback to p9_virtio_cancelled().
> >
> > 4. We reference the req object below in p9_virtio_zc_request(), and then
> > hit a refcnt underflow when we again try to drop a reference in the
> > reterr path.
> >
> > Let's fix this by only dropping that reference if we're not exiting due to
> > -ERESTARTSYS, as the assumption is that the reference will instead be
> > dropped later when we flush the request.
>
> Thanks for the patch!
> I hadn't forgotten about it, I'm just not convinced this is the only
> problem we have around these refs or how to fix that, and haven't had
> time to look in details...
> looking again now I think we can sum it up to a p9_client_flush problem
> instead: basically dropping your 3. instead of dropping the 2.
>
> If the request really was cancelled we'll get the flush reply before the
> response and that'll drop the second ref in the cancelled callback,
> which would also blow up, but afaik qemu doesn't implement this so this
> code path so this was never tested.
>
> And this also made me notice our cancelled callbacks are all over the
> place with rdma and xen not doing anything about the ref, so let's make
> that ref dropping common, so we can remove the virtio callback
> altogether - I'm now starting to doubt if any server actually implement
> cancelling flushes, it might be worth testing with a synthetic fd mount
> as syzcaller does..
>
> So something like that (untested)
> ---
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 5cd94721d974..51ce3bd4aece 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -616,9 +616,10 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
> if (READ_ONCE(oldreq->status) == REQ_STATUS_SENT) {
> if (c->trans_mod->cancelled)
> c->trans_mod->cancelled(c, oldreq);
> + /* reply won't come anymore, drop the "receive" ref */
> + p9_req_put(client, req);
> }
>
> - p9_req_put(c, req);
> return 0;
> }
>
> diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
> index 196060dc6138..5184730a2639 100644
> --- a/net/9p/trans_fd.c
> +++ b/net/9p/trans_fd.c
> @@ -737,8 +737,6 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
> WRITE_ONCE(req->status, REQ_STATUS_FLSHD);
> spin_unlock(&m->req_lock);
>
> - p9_req_put(client, req);
> -
> return 0;
> }
>
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 0b8086f58ad5..1c1fd9fbecc9 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -195,13 +195,6 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
> return 1;
> }
>
> -/* Reply won't come, so drop req ref */
> -static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req)
> -{
> - p9_req_put(client, req);
> - return 0;
> -}
> -
> /**
> * pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer,
> * this takes a list of pages.
> @@ -793,7 +786,6 @@ static struct p9_trans_module p9_virtio_trans = {
> .request = p9_virtio_request,
> .zc_request = p9_virtio_zc_request,
> .cancel = p9_virtio_cancel,
> - .cancelled = p9_virtio_cancelled,
> /*
> * We leave one entry for input and one entry for response
> * headers. We also skip one more entry to accommodate, address
>
> ---
>
> I'll try to find time to play with it a bit more overnight then send the
> patch proper and ask syzbot if that fixes this one:
> https://lkml.kernel.org/r/000000000000c18c15061dcff478@xxxxxxxxxx
>
> But if you could also test that'd be great.
> Shall I keep you as the author for the above, or would you rather just
> be reported-by given the patch is different enough?
I appreciate you asking -- I'll leave it up to whatever you feel comfortable
doing as the maintainer. Happy to keep my SoB on if the patch is close enough
but whatever you decide to do is fine with me.
Thanks,
David
Attachment:
signature.asc
Description: PGP signature