[PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Yizhou Zhao]

handle_rerror() copies the variable-length error string of a zero-copy
RERROR response from the receive pages into the request's static response
buffer.  The amount copied is bounded by P9_ZC_HDR_SZ, so the data can
span at most two pages, but the helper is not told how many receive pages
were actually mapped.

If a malicious or broken virtio 9p device reports an RERROR length that
exceeds the remaining bytes in the first mapped receive page, the error
string is treated as crossing into a second page.  When only one receive
page was mapped, handle_rerror() still advances the page pointer and
dereferences the next entry, reading past the allocated in_pages array.

Pass the number of mapped receive pages to handle_rerror().  If the error
string would cross a page boundary but only one page is available, copy the
bytes that fit in that page and leave the response truncated, matching the
existing behavior for overlong RERROR messages.  Otherwise continue with
the second-page copy as before.

Fixes: f615625a44c4 ("9p: handling Rerror without copy_from_iter_full()")
Cc: stable@vger.kernel.org
Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
Reported-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
Reported-by: Ao Wang <wangao@seu.edu.cn>
Reported-by: Xuewei Feng <fengxw06@126.com>
Reported-by: Qi Li <qli01@tsinghua.edu.cn>
Reported-by: Ke Xu <xuke@tsinghua.edu.cn>
Assisted-by: GLM:GLM-5.1
Signed-off-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
---
 net/9p/trans_virtio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 4cdab7094b27..a71ce8870c53 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -377,7 +377,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
 }
 
 static void handle_rerror(struct p9_req_t *req, int in_hdr_len,
-			  size_t offs, struct page **pages)
+			  size_t offs, struct page **pages, int in_nr_pages)
 {
 	unsigned size, n;
 	void *to = req->rc.sdata + in_hdr_len;
@@ -398,6 +398,8 @@ static void handle_rerror(struct p9_req_t *req, int in_hdr_len,
 	n = PAGE_SIZE - offs;
 	if (size > n) {
 		memcpy_from_page(to, *pages++, offs, n);
+		if (in_nr_pages < 2)
+			return;
 		offs = 0;
 		to += n;
 		size -= n;
@@ -535,7 +537,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 	// RERROR needs reply (== error string) in static data
 	if (READ_ONCE(req->status) == REQ_STATUS_RCVD &&
 	    unlikely(req->rc.sdata[4] == P9_RERROR))
-		handle_rerror(req, in_hdr_len, offs, in_pages);
+		handle_rerror(req, in_hdr_len, offs, in_pages, in_nr_pages);
 
 	/*
 	 * Non kernel buffers are pinned, unpin them
-- 
2.43.0

Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Dominique Martinet]

Yizhou Zhao wrote on Sun, Jun 07, 2026 at 10:06:01PM +0800:
> handle_rerror() copies the variable-length error string of a zero-copy
> RERROR response from the receive pages into the request's static response
> buffer.  The amount copied is bounded by P9_ZC_HDR_SZ, so the data can
> span at most two pages, but the helper is not told how many receive pages
> were actually mapped.
> 
> If a malicious or broken virtio 9p device reports an RERROR length that
> exceeds the remaining bytes in the first mapped receive page, the error
> string is treated as crossing into a second page.  When only one receive
> page was mapped, handle_rerror() still advances the page pointer and
> dereferences the next entry, reading past the allocated in_pages array.

I'm not sure that can actually happen:
If there was an "in page" (if there wasn't this is all noop anyway and
data was written directly to req->rc.sdata), then req->rc.size amount of
data was received into the pages, so if it does happen to span over a
page boundary then there are at least two pages and we don't need to
double-check here.

Christian, do you agree with me this patch is not required?

> Pass the number of mapped receive pages to handle_rerror().  If the error
> string would cross a page boundary but only one page is available, copy the
> bytes that fit in that page and leave the response truncated, matching the
> existing behavior for overlong RERROR messages.  Otherwise continue with
> the second-page copy as before.


-- 
Dominique Martinet | Asmadeus

Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Christian Schoenebeck]

On Sunday, 13 September 2026 11:45:13 CEST Dominique Martinet wrote:
> Yizhou Zhao wrote on Sun, Jun 07, 2026 at 10:06:01PM +0800:
> > handle_rerror() copies the variable-length error string of a zero-copy
> > RERROR response from the receive pages into the request's static response
> > buffer.  The amount copied is bounded by P9_ZC_HDR_SZ, so the data can
> > span at most two pages, but the helper is not told how many receive pages
> > were actually mapped.
> > 
> > If a malicious or broken virtio 9p device reports an RERROR length that
> > exceeds the remaining bytes in the first mapped receive page, the error
> > string is treated as crossing into a second page.  When only one receive
> > page was mapped, handle_rerror() still advances the page pointer and
> > dereferences the next entry, reading past the allocated in_pages array.
> 
> I'm not sure that can actually happen:
> If there was an "in page" (if there wasn't this is all noop anyway and
> data was written directly to req->rc.sdata), then req->rc.size amount of
> data was received into the pages, so if it does happen to span over a
> page boundary then there are at least two pages and we don't need to
> double-check here.
> 
> Christian, do you agree with me this patch is not required?

req->rc.size is coming from virtio device's used-ring len, and that is written 
by 9p server, and this server-written value is not verified anywhere against 
the real sizes of the descriptors that guest provided, neither by the kernel's 
virtio subsystem, nor by 9p client.

So the patch's premise is: 9p server is untrusted and lying about the written 
used-ring len. In that case, yes, it could crash the client.

Should you pick the patch? You can, it does fix what it claims to do, but this 
is one of those project-policy / maintainer's personal opinion dependent 
issues than a real-life problem fix IMHO:

- Malicious 9p server: of course server can lie about used-ring len and crash 
the Linux guest this way, but server could do that anyway, at any time. This 
is a virtio specific issue, 9p server is also Linux guest's host (i.e. it 
fully controls guest's memory, can simply kill/shutdown at any time, same 
outcome).

- Broken 9p server: that's an argument that might justify to pick the patch, 
i.e. the wrong length was unintentionally written by server, everything else 
server is behaving fine - very unlikely though.

P.S. Remarkable backlog processing today! :)

/Christian

Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Dominique Martinet]

Christian Schoenebeck wrote on Sun, Sep 13, 2026 at 08:09:05PM +0200:
> > I'm not sure that can actually happen:
> > If there was an "in page" (if there wasn't this is all noop anyway and
> > data was written directly to req->rc.sdata), then req->rc.size amount of
> > data was received into the pages, so if it does happen to span over a
> > page boundary then there are at least two pages and we don't need to
> > double-check here.
> > 
> > Christian, do you agree with me this patch is not required?
> 
> req->rc.size is coming from virtio device's used-ring len, and that is written 
> by 9p server, and this server-written value is not verified anywhere against 
> the real sizes of the descriptors that guest provided, neither by the kernel's 
> virtio subsystem, nor by 9p client.

Wait, is it?
req->rc.size is written in req_done() and comes from
virtqueue_get_buf(), I assume that was the actual data obtained from the
transport (e.g. akin to read() return value, not what is in the payload)
Or can the server lie about this?

If we can't trust req->rc.size I believe the p9pdu processing will also
read past the end of buffer so there are much bigger "problems"


With that said, I agree with your assessment that qemu is mostly
trusted, and don't want to spend too much effort on this (unless
something like me losing my job happens and I suddently find a lot of
free time :P), so if you (or someone) can confirm rc.size isn't trusted
then let's drop this here.

FWIW I said "mostly trusted" because lately we've had things like black
box hypervisors (I don't remember the exact name) where guest VMs memory
is encrypted and can't be accessed even by the host: sure the server
could still *crash* a guest, but it shouldn't be able to get arbitrary
write/read primitives, so if someone wants to spend effort fixing such
bugs I think it's welcome and I'll be happy to take patches.
(although it should probably start at getting a way to trust the size
e.g. clamp it to whatever pages we fed to the server rather than check here)

This pretty much aligns with what Jürgen said for xen recently here
=====
https://lore.kernel.org/r/931b763d-2ee6-4fbf-9222-c5b75f35ec47@suse.com
> Especially with driver domains malicious backends are a thing. They should
> only be capable to deliver wrong or no data to the frontend, but ideally
> the frontend should not trust the backend.
> 
> Any work towards that goal is to be supported IMHO, and there are already
> frontends listed in Xen's support statement following this rule, so any
> violation of that principle in those frontends will be regarded to be a
> security issue worth an XSA.
====

> P.S. Remarkable backlog processing today! :)

Thanks! I _think_ I got to at reply to each mail I wanted to look at
overnight, but I'm sure I missed some, so if you have something in
progress that you want me to look at feel free to ping me again
(and someday we'll be able to get through the virtio backend page
mapping rework started in December[1] (Christoph Hellwig didn't forget and
pinged us in May...) / your msize limit lifting work[2]... someday...
But that should probably get priority over chasing untrusted servers)
(The links are for myself more than anything else, my working memo is in
flight mails)

[1] 20251214-virtio_trans_iter-v2-1-f7f7072e8c15@codewreck.org
[2] https://lore.kernel.org/all/cover.1657920926.git.linux_oss@crudebyte.com/

Cheers,
-- 
Dominique

Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Christian Schoenebeck]

On Monday, 14 September 2026 01:36:49 CEST Dominique Martinet wrote:
> Christian Schoenebeck wrote on Sun, Sep 13, 2026 at 08:09:05PM +0200:
> > > I'm not sure that can actually happen:
> > > If there was an "in page" (if there wasn't this is all noop anyway and
> > > data was written directly to req->rc.sdata), then req->rc.size amount of
> > > data was received into the pages, so if it does happen to span over a
> > > page boundary then there are at least two pages and we don't need to
> > > double-check here.
> > > 
> > > Christian, do you agree with me this patch is not required?
> > 
> > req->rc.size is coming from virtio device's used-ring len, and that is
> > written by 9p server, and this server-written value is not verified
> > anywhere against the real sizes of the descriptors that guest provided,
> > neither by the kernel's virtio subsystem, nor by 9p client.
> 
> Wait, is it?
> req->rc.size is written in req_done() and comes from
> virtqueue_get_buf(), I assume that was the actual data obtained from the
> transport (e.g. akin to read() return value, not what is in the payload)
> Or can the server lie about this?

net/9p/trans_virtio.c:

static void req_done(struct virtqueue *vq)
{
    ...
    unsigned int len;
    struct p9_req_t *req;
    ...
    while ((req = virtqueue_get_buf(chan->vq, &len)) != NULL) {
    ...
        if (len) {
	req->rc.size = len;
	p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
        }
        ...
    }
    ...
}

drivers/virtio/virtio_ring.c:

virtqueue_get_buf() ->
    virtqueue_get_buf_ctx() ->
        VIRTQUEUE_CALL() ->
            // for "split" ring type :
            virtqueue_get_buf_ctx_split(
                vring_virtqueue *vq,
                unsigned int *len,
                void **ctx)
            {
                ...
                *len = vring_read_split_used_len(vq, last_used);
                ...
            }

static inline u32 vring_read_split_used_len(const struct vring_virtqueue *vq,
                                            u16 idx)
{
    return virtio32_to_cpu(vq->vq.vdev,
                           READ_ONCE(vq->split.vring.used->ring[idx].len));
}

And `len` field is just shared DMA memory written by server. The len value is 
not questioned anywhere in the call stack, it is taken as-is.

This is for "split" ring type which QEMU's 9p virtio device is always using 
(as QEMU's virtio device is not advertising "packed" ring type). Linux 9p 
client is agnostic about the two, but it does not matter either way, because 
even with "packed" ring type the call stack would be slightly different, the 
len field would be 32-bit instead of 16-bit, but its value was still not 
questioned anywhere.

> If we can't trust req->rc.size I believe the p9pdu processing will also
> read past the end of buffer so there are much bigger "problems"
> 
> With that said, I agree with your assessment that qemu is mostly
> trusted, and don't want to spend too much effort on this (unless
> something like me losing my job happens and I suddently find a lot of
> free time :P), so if you (or someone) can confirm rc.size isn't trusted
> then let's drop this here.

Surprisingly though, it is not as bad as one might think first. OOB should be 
limited to the page walk in handle_rerror() - this patch; and to pdu parse for 
the 9p2000.u error string (i.e. Twrite ZC -> handle_rerror() returns early on
!pages -> p9_check_errors() then OOB there at:

  ...
  p9pdu_readf(&req->rc, c->proto_version, "s?d", &ename, &ecode);
  ...

All others should be fine actually, since:

- Non zero-copy 9p request types are safe (checked against capacity), so it 
boils down to Tread/Treaddir/Twrite request types responses as only possible 
cases:

- Tread/Treaddir/Twrite returning *non-error* response (and server lying about 
len): all count validated by client already -> safe.

- So it is really just 9p2000.u Rerror returned for either Tread/Treaddir/
Twrite: Tread and Treaddir handled by patch -> then safe, Twrite as outlined 
above being the only open candidate left and that could (additional to this 
patch) be easily fixed by just moving the P9_ZC_HDR_SZ clamp before the return 
on !pages in handle_rerror() happens.

Or:

You could simply pass the bad card on to the virtio subsystem maintainers, and 
tell them to clamp used-ring element's `len` field against the guest provided 
descriptor length, then req->rc.size would be trusted - no validation to be 
done on 9p side. But even then I can imagine that this would be implemented by 
virtio maintainers as an opt-in solution for drivers like "please clamp this 
for me" to avoid breaking certain legacy devices.

> FWIW I said "mostly trusted" because lately we've had things like black
> box hypervisors (I don't remember the exact name) where guest VMs memory
> is encrypted and can't be accessed even by the host: sure the server
> could still *crash* a guest, but it shouldn't be able to get arbitrary
> write/read primitives, so if someone wants to spend effort fixing such
> bugs I think it's welcome and I'll be happy to take patches.

"Confidential Guest Support" is the term. Wouldn't change the picture here 
though, as this is about a host triggered DoS. Host could still kill the guest 
at any time.

> > P.S. Remarkable backlog processing today! :)
> 
> Thanks! I _think_ I got to at reply to each mail I wanted to look at
> overnight, but I'm sure I missed some, so if you have something in
> progress that you want me to look at feel free to ping me again
> (and someday we'll be able to get through the virtio backend page
> mapping rework started in December[1] (Christoph Hellwig didn't forget and
> pinged us in May...) / your msize limit lifting work[2]... someday...
> But that should probably get priority over chasing untrusted servers)
> (The links are for myself more than anything else, my working memo is in
> flight mails)

The negative dentries patches come to my mind which you might have forgotten; 
I think v7 is the latest one on these:

https://lore.kernel.org/all/cover.1779355927.git.repk@triplefau.lt/

/Christian

Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Dominique Martinet]

Christian Schoenebeck wrote on Mon, Sep 14, 2026 at 06:01:04AM +0200:
> And `len` field is just shared DMA memory written by server. The len value is 
> not questioned anywhere in the call stack, it is taken as-is.
> 
> This is for "split" ring type which QEMU's 9p virtio device is always using 
> (as QEMU's virtio device is not advertising "packed" ring type). Linux 9p 
> client is agnostic about the two, but it does not matter either way, because 
> even with "packed" ring type the call stack would be slightly different, the 
> len field would be 32-bit instead of 16-bit, but its value was still not 
> questioned anywhere.

Thank you for the legwork!

> > If we can't trust req->rc.size I believe the p9pdu processing will also
> > read past the end of buffer so there are much bigger "problems"
> > 
> > With that said, I agree with your assessment that qemu is mostly
> > trusted, and don't want to spend too much effort on this (unless
> > something like me losing my job happens and I suddently find a lot of
> > free time :P), so if you (or someone) can confirm rc.size isn't trusted
> > then let's drop this here.
> 
> Surprisingly though, it is not as bad as one might think first.
> [...]
> Or:
> You could simply pass the bad card on to the virtio subsystem maintainers, and 
> tell them to clamp used-ring element's `len` field against the guest provided 
> descriptor length, then req->rc.size would be trusted - no validation to be 
> done on 9p side. But even then I can imagine that this would be implemented by 
> virtio maintainers as an opt-in solution for drivers like "please clamp this 
> for me" to avoid breaking certain legacy devices.

Given how surprised I found this I could imagine other virtio users to
incorrectly assume the same, so I think it's worth bringing up even if
it doesn't end up implemented (we could actually probably cap this on
the 9p side if we wanted to anyway)

I think truncating rc.size is worth the effort even if most of the paths
are correct, and more appropriate than this current patch, so let's drop
this patch - thank you nevertheless Yizhou Zhao for starting the
discussion.
I'm out of steam though so will take some time to send the
virtualization list a mail.. :)


> > FWIW I said "mostly trusted" because lately we've had things like black
> > box hypervisors (I don't remember the exact name) where guest VMs memory
> > is encrypted and can't be accessed even by the host: sure the server
> > could still *crash* a guest, but it shouldn't be able to get arbitrary
> > write/read primitives, so if someone wants to spend effort fixing such
> > bugs I think it's welcome and I'll be happy to take patches.
> 
> "Confidential Guest Support" is the term. Wouldn't change the picture here 
> though, as this is about a host triggered DoS. Host could still kill the guest 
> at any time.

Hm, yeah, I was confused and considered this as a oob write but it's the
"reading a page that doesn't exist" that fails, so it's not likely to be
too bad even if it does happen.

> The negative dentries patches come to my mind which you might have forgotten; 
> I think v7 is the latest one on these:
> 
> https://lore.kernel.org/all/cover.1779355927.git.repk@triplefau.lt/

I'm not sure why I didn't reply to that thread back then, but it's been
picked up in 7.2 :)

Cheers,
-- 
Dominique Martinet | Asmadeus