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
| ||
|
Date: Tue, 10 Jul 2018 10:15:00 +0200 From: Tomas Bortoli <tomasbortoli@...il.com> To: Dominique Martinet <asmadeus@...ewreck.org> Cc: ericvh@...il.com, rminnich@...dia.gov, lucho@...kov.net, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, syzkaller@...glegroups.com, v9fs-developer@...ts.sourceforge.net, davem@...emloft.net, Al Viro <viro@...IV.linux.org.uk> Subject: Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length 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 has >> 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 read > 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 == m->rc.capacity)) { > p9_debug(P9_DEBUG_TRANS, "got new packet\n"); > + m->req->rc->size = m->rc.offset; > spin_lock(&m->client->lock); > if (m->req->status != REQ_STATUS_ERROR) > status = 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 whatever > 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.
Powered by blists - more mailing lists