Messages in this thread | | | From | Christian Schoenebeck <> | Subject | Re: [PATCH 1/2] 9p/xen: check logical size for buffer size | Date | Mon, 21 Nov 2022 17:35:56 +0100 |
| |
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?
> 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; >
| |