Messages in this thread | | | Date | Mon, 21 Nov 2022 15:01:54 -0800 (PST) | From | Stefano Stabellini <> | Subject | Re: [PATCH 1/2] 9p/xen: check logical size for buffer size |
| |
On Mon, 21 Nov 2022, Christian Schoenebeck wrote: > On Friday, November 18, 2022 2:55:41 PM CET 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@codewreck.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->status = REQ_STATUS_ERROR; > > + goto recv_error; > > + } > > + > > Looks good (except of s/rreq/req/ mentioned by Stefano already). > > > memcpy(&req->rc, &h, sizeof(h)); > > Is that really OK? > > 1. `h` is of type xen_9pfs_header and declared as packed, whereas `rc` is of > type p9_fcall not declared as packed. > > 2. Probably a bit dangerous to assume the layout of xen_9pfs_header being in > sync with the starting layout of p9_fcall without any compile-time > assertion?
You are right. It would be better to replace the memcpy with:
req->rc.size = h.size; req->rc.id = h.id; req->rc.tag = h.tag;
| |