Re: [V9fs-developer] [PATCH] p9_check_errors() validate PDU length

[Tomas Bortoli <tomasbortoli@gmail.com>]

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.