Re: [PATCH 1/5] 9p/virtio: clamp the reply size of a zero-copy request

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The clamp added to p9_virtio_zc_request()
  (net/9p/trans_virtio.c) is `req->rc.size = umin(req->rc.size,…

--

9p/virtio: clamp the reply size of a zero-copy request

This commit clamps req->rc.size in p9_virtio_zc_request() to the total
size of the buffers posted to the device (in_hdr_len + inlen). The clamp
happens before handle_rerror() runs. The aim is to stop oversized error
replies to zero-copy requests from being parsed past the posted buffers.

Closes: https://lore.kernel.org/all/20260607140603.24342-1-zhaoyz24@xxxxxxxxxxxxxxxxxxxxx/

> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 9fa63e3ebf673..53046668c16b1 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -537,10 +537,14 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
>  	 * threads - echoes to wmb() in the callback
>  	 */
>  	smp_rmb();
> -	// 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);
> +	if (READ_ONCE(req->status) == REQ_STATUS_RCVD) {
> +		// The device may report more than the buffers we posted
> +		req->rc.size = umin(req->rc.size, in_hdr_len + inlen);
> +
> +		// RERROR needs reply (== error string) in static data
> +		if (unlikely(req->rc.sdata[4] == P9_RERROR))
> +			handle_rerror(req, in_hdr_len, offs, in_pages);
> +	}

[Severity: High]
Can an oversized P9_RLERROR reply to a zero-copy TREAD still make
p9_check_errors() read past the end of rc.sdata?

For a zero-copy TREAD, p9_client_read_once() passes in_hdr_len = 11 and
inlen = rsize, which can be up to msize - 24. So the new bound can be
much larger than rc.capacity. rc.sdata is a P9_ZC_HDR_SZ (4096) byte
allocation, and only its first 11 bytes are posted to the device.

The only truncation to the rc.sdata size is in handle_rerror():

    if (unlikely(req->rc.size > P9_ZC_HDR_SZ))
        req->rc.size = P9_ZC_HDR_SZ;

That runs only when sdata[4] == P9_RERROR. p9_check_errors() skips its
capacity check for zc replies, and it handles both error types:

    if (req->rc.size > req->rc.capacity && !req->rc.zc) {
    ...
    if (type != P9_RERROR && type != P9_RLERROR)
        return 0;

On a non-.L client, both types are parsed with "s?d". The client can be
non-.L because of the mount option. It can also be non-.L because the
device answers RVERSION with "9P2000" or "9P2000.u", which
p9_client_version() accepts.

Here is the case. The device answers a zero-copy TREAD with an 11-byte
header that has type P9_RLERROR and size = 11 + inlen, plus a large
string length, and it reports used len = 11 + inlen. Then:

    p9_virtio_zc_request()
        rc.size stays 11 + inlen, handle_rerror() is skipped
    p9_check_errors()
        p9pdu_readf(&req->rc, ..., "s?d", &ename, &ecode)
            pdu_read()
                len = min(pdu->size - pdu->offset, size);
                memcpy(data, &pdu->sdata[pdu->offset], len);

pdu_read() is bounded by pdu->size, not pdu->capacity. Could that read
tens of KiB past the 4096-byte rc.sdata object?

With legacy 9P2000, the string then goes to p9_errstr2errno(), which
calls pr_err() for unknown error strings. Would that put adjacent slab
contents into the kernel log?

The commit message says the patch prevents p9_check_errors() from
parsing "the error string of an oversized error reply past the end of
rc.sdata". This case still seems reachable after the patch.

Clamping rc.size to rc.capacity for every zc reply would break
successful reads. p9_client_read_once() bounds the received count by
rc.size via "D". Would it work better to give P9_RLERROR the same
truncation and copy that P9_RERROR gets in handle_rerror()? Another
option is to bound the error-string parse in p9_check_errors() by
rc.capacity for zc replies.

>  
>  	/*
>  	 * Non kernel buffers are pinned, unpin them

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109105251.2545764-1-lian%40is.s.u-tokyo.ac.jp