linux-kernel - Re: [PATCH 1/2] 9p/xen: check logical size for buffer size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2211181713420.1049131@ubuntu-linux-20-04-desktop>
Date:   Fri, 18 Nov 2022 17:51:46 -0800 (PST)
From:   Stefano Stabellini <sstabellini@...nel.org>
To:     Dominique Martinet <asmadeus@...ewreck.org>
cc:     Stefano Stabellini <sstabellini@...nel.org>,
        GUO Zihua <guozihua@...wei.com>, linux_oss@...debyte.com,
        v9fs-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] 9p/xen: check logical size for buffer size

On Fri, 18 Nov 2022, Dominique Martinet wrote:
> trans_xen did not check the data fits into the buffer before copying
> from the xen ring, but we probably should.
> Add a check that just skips the request and return an error to
> userspace if it did not fit
> 
> Signed-off-by: Dominique Martinet <asmadeus@...ewreck.org>
> ---
> 
> This comes more or less as a follow up of a fix for trans_fd:
> https://lkml.kernel.org/r/20221117091159.31533-1-guozihua@huawei.com
> Where msize should be replaced by capacity check, except trans_xen
> did not actually use to check the size fits at all.
> 
> While we normally trust the hypervisor (they can probably do whatever
> they want with our memory), a bug in the 9p server is always possible so
> sanity checks never hurt, especially now buffers got drastically smaller
> with a recent patch.
> 
> My setup for xen is unfortunately long dead so I cannot test this:
> Stefano, you've tested v9fs xen patches in the past, would you mind
> verifying this works as well?
> 
>  net/9p/trans_xen.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index b15c64128c3e..66ceb3b3ae30 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -208,6 +208,14 @@ static void p9_xen_response(struct work_struct *work)
>  			continue;
>  		}
>  
> +		if (h.size > req->rc.capacity) {
> +			dev_warn(&priv->dev->dev,
> +				 "requested packet size too big: %d for tag %d with capacity %zd\n",
> +		                 h.size, h.tag, rreq->rc.capacity);

"req" instead of "rreq"

I made this change and tried the two patches together. Unfortunately I
get the following error as soon as I try to write a file:

/bin/sh: can't create /mnt/file: Input/output error


Next I reverted the second patch and only kept this patch. With that, it
worked as usual. It looks like the second patch is the problem. I have
not investigated further.



> +			req->status = REQ_STATUS_ERROR;
> +			goto recv_error;
> +		}
> +
>  		memcpy(&req->rc, &h, sizeof(h));
>  		req->rc.offset = 0;
>  
> @@ -217,6 +225,7 @@ static void p9_xen_response(struct work_struct *work)
>  				     masked_prod, &masked_cons,
>  				     XEN_9PFS_RING_SIZE(ring));
>  
> +recv_error:
>  		virt_mb();
>  		cons += h.size;
>  		ring->intf->in_cons = cons;
> -- 
> 2.38.1
>