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