lkml.org 
[lkml]   [2018]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length
From
Date
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.


\
 
 \ /
  Last update: 2018-07-10 10:15    [W:0.411 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site