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

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

[Tomas Bortoli]

p9_check_errors() does not validate the size of the PDU read
in p9_parse_header(). Any size can be passed, provoking out-of-bound reads.

Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
---
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.

 net/9p/client.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index 40f7c47f2f74..5b161b576b8a 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -520,10 +520,13 @@ EXPORT_SYMBOL(p9_parse_header);
 static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
 {
 	int8_t type;
+	int32_t size;
 	int err;
 	int ecode;
 
-	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
+	err = p9_parse_header(req->rc, &size, &type, NULL, 0);
+	if (size > req->rc->capacity)
+		return -EINVAL;
 	/*
 	 * dump the response from server
 	 * This should be after check errors which poplulate pdu_fcall.
-- 
2.11.0

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

[Dominique Martinet]

(extra Cc: Al Viro, as it's kind of the continuation of the other
thread)

Tomas Bortoli wrote on Tue, Jul 10, 2018:
> p9_check_errors() does not validate the size of the PDU read
> in p9_parse_header(). Any size can be passed, provoking out-of-bound reads.

cf. what I just said in other patch, I think this check needs to be
moved up to p9_parse_header as p9_check_zc_error has the same problem.

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)

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 3d414acb7015..2649b2ebf961 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -320,6 +320,7 @@ recv_done(struct ib_cq *cq, struct ib_wc *wc)
 	if (wc->status != IB_WC_SUCCESS)
 		goto err_out;
 
+	c->rc->size = wc->byte_len;
 	err = p9_parse_header(c->rc, NULL, NULL, &tag, 1);
 	if (err)
 		goto err_out;
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 05006cbb3361..fc6dc9ca86a4 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -159,8 +159,10 @@ static void req_done(struct virtqueue *vq)
 		spin_unlock_irqrestore(&chan->lock, flags);
 		/* Wakeup if anyone waiting for VirtIO ring space. */
 		wake_up(chan->vc_wq);
-		if (len)
+		if (len) {
+			req->rc->size = len;
 			p9_client_cb(chan->client, req, REQ_STATUS_RCVD);
+		}
 	}
 }
-- 
Dominique Martinet | Asmadeus

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

[piaojun]

Hi Tomas,

On 2018/7/10 6:43, Tomas Bortoli wrote:
> p9_check_errors() does not validate the size of the PDU read
> in p9_parse_header(). Any size can be passed, provoking out-of-bound reads.
> 
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
> 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.
> 
>  net/9p/client.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 40f7c47f2f74..5b161b576b8a 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -520,10 +520,13 @@ EXPORT_SYMBOL(p9_parse_header);
>  static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  {
>  	int8_t type;
> +	int32_t size;
>  	int err;
>  	int ecode;
>  
> -	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
> +	err = p9_parse_header(req->rc, &size, &type, NULL, 0);
> +	if (size > req->rc->capacity)
> +		return -EINVAL;

I think we should print some error logs here to remind uppper users.

Thanks,
Jun

>  	/*
>  	 * dump the response from server
>  	 * This should be after check errors which poplulate pdu_fcall.
> 

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

[Dominique Martinet]

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

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

[Tomas Bortoli]

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.

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

[Dominique Martinet]

Tomas Bortoli wrote on Tue, Jul 10, 2018:
> > 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.

That's precisely the problem, none of the transports actually write in
the pdu how much has been read.
Instead, we blindly trust how much the client says it has written and
p9_parse_header will write pdu->size which is then used as the 'actual'
PDU length.


With the three lines I added in each file all transports will store the
PDU length in pdu->size so my following suggestion would be to just
check that in p9_parse_header the length read from the packet match that
of the received data and flag any packet where this is wrong as invalid.

The only problem with that is that TCP will use p9_parse_header with a
partial packet to know how much it needs to read, so that modification
needs actual testing (hence my question below) as it is more intrusive
than a simple boundary check.

I'd suggest implementing some logic like already here with the if
(pdu->size == 0) -- only check if pdu->size was previously set, and set
it if it wasn't... But feel free to try something else.

With that done I don't expect more problems but if I do it there's
little point in making you do it as well ;)


> > 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.

Ok, let's get you a working setup for TCP then.
You need to install a 9p server, the two I'm aware of are diod and
nfs-ganesha (I'm using ganesha); once you have a working config you can
mount with something like this:
 mount -t 9p -o aname=path,trans=fd <server-ip> <mount-point>

Once you have that you should be able to fiddle with things until it
works as expected.
(for virtio I use qemu, that's probably easy to test as well; for RDMA
you can use rxe to setup a virtual interface (with rxe_cfg) and then use
either diod or ganesha again but the setup is more complicated so it's
OK to leave that aside for now)

> 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.

There's a saying about giving a fish or teaching how to fish, so let's
have you try if you're motivated :)

I don't think much is left if you can sew the pieces together, the most
difficult part will probably to set the test environment up if you want
to do this properly.


Please ask if you run into any trouble,
-- 
Dominique Martinet | Asmadeus

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

[jiangyiwen]

On 2018/7/10 6:43, Tomas Bortoli wrote:
> p9_check_errors() does not validate the size of the PDU read
> in p9_parse_header(). Any size can be passed, provoking out-of-bound reads.
> 
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com
> ---
> 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.
> 
>  net/9p/client.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 40f7c47f2f74..5b161b576b8a 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -520,10 +520,13 @@ EXPORT_SYMBOL(p9_parse_header);
>  static int p9_check_errors(struct p9_client *c, struct p9_req_t *req)
>  {
>  	int8_t type;
> +	int32_t size;
>  	int err;
>  	int ecode;
>  
> -	err = p9_parse_header(req->rc, NULL, &type, NULL, 0);
> +	err = p9_parse_header(req->rc, &size, &type, NULL, 0);
> +	if (size > req->rc->capacity)
> +		return -EINVAL;

Yes, currently 9p client is lacking of properly check for data from server.
Maybe we should have a bigger patch to solve this problem.

>  	/*
>  	 * dump the response from server
>  	 * This should be after check errors which poplulate pdu_fcall.
>