From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomas Bortoli Subject: Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length Date: Tue, 10 Jul 2018 10:15:00 +0200 Message-ID: References: <20180709224323.20597-1-tomasbortoli@gmail.com> <20180709235403.GB19917@nautica> <20180710022819.GA19285@nautica> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: ericvh@gmail.com, rminnich@sandia.gov, lucho@ionkov.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller@googlegroups.com, v9fs-developer@lists.sourceforge.net, davem@davemloft.net, Al Viro To: Dominique Martinet Return-path: In-Reply-To: <20180710022819.GA19285@nautica> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 07/10/2018 04:28 AM, Dominique Martinet wrote: > Tomas Bortoli wrote on Tue, Jul 10, 2018: >> As suggested by Dominique: >> https://lkml.org/lkml/2018/7/9/688 >> Such check is not enough as it will prevent to read more than how it h= as >> been allocated but it won't prevent to read more than how it has been = read >> So this patch will require some more changes to prevent bad sizes. > Sorry, I'm the one who suggested to put a note after the commit message= > and I didn't see it. > > Let's get the proper fix right away, it's not much further. I agree. > >> Also, they really need to check against the actual read size, not just= >> capacity. >> For virtio/rdma, something like this ought to fix pdu->size, then >> p9_parse_header can just never overwrite it (untested but it's useless= >> on its own, I'll test the full patch with the parse header change) > I actually took the time to test a bit; I had only suggested something > for virtio/rdma because I had assumed trans_fd (the socket transport > actually used by syzbot) was setting the length in the fcall, but I rea= d > that code too fast this morning and it is not (it only sets the size in= > its private struct) > > Something like that ought to work for trans_fd: > diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c > index 588bf88c3305..9f3ce370c685 100644 > --- a/net/9p/trans_fd.c > +++ b/net/9p/trans_fd.c > @@ -369,6 +370,7 @@ static void p9_read_work(struct work_struct *work) > */ > if ((m->req) && (m->rc.offset =3D=3D m->rc.capacity)) { > p9_debug(P9_DEBUG_TRANS, "got new packet\n"); > + m->req->rc->size =3D m->rc.offset; > spin_lock(&m->client->lock); > if (m->req->status !=3D REQ_STATUS_ERROR) > status =3D REQ_STATUS_RCVD; > --- > > This however gets more complicated once you start factoring in that > change I suggested about p9_parse_header not setting size (and checking= > size) because trans_fd relies on it; so I'm not sure how we should > proceed. Mmh, me neither. I don't see where the *actual* PDU length is stored. > > Do you have a working 9p tcp server to test changes are valid, or are > you only working off the syzbot reproducer? No, I was just using the reproducer to test. > In the first place, are you willing to take the time to do that bigger > fix? Yes. > > At this point I can either help you get a working setup and let you do > the rest, or just finish the bigger patch myself and add you as whateve= r > tag you feel comfortable with (persumably Signed-off-by) > > Thanks again for starting this, For me both ways are good. Signed-off-by will be good. You know for sure more than me about 9p as I started delving it for the first time yesterday. I can also make and test a patch but I'll need to understand more about it. Let me know if you find out more.