Messages in this thread Patch in this message | | | Date | Tue, 10 Jul 2018 04:28:19 +0200 | From | Dominique Martinet <> | Subject | Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length |
| |
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.
> 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.
Do you have a working 9p tcp server to test changes are valid, or are you only working off the syzbot reproducer? In the first place, are you willing to take the time to do that bigger fix?
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, -- Dominique Martinet | Asmadeus
| |