lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;

\
 
 \ /
  Last update: 2022-11-22 00:02    [W:0.689 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site