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

Tomas Bortoli wrote on Thu, Jul 12, 2018:

+ Cc: Andrew Morton [off-list ref]

On 07/12/2018 01:43 PM, Dominique Martinet wrote:

quoted

Tomas Bortoli wrote on Thu, Jul 12, 2018:

quoted

This patch adds checks to the p9_parse_header() function to
verify that the length found within the header coincides with the actual
length of the PDU. Furthermore, it checks that the length stays within the
acceptable range. To do this the patch brings the actual length of the PDU
from the different transport layers (rdma and virtio). For TCP (trans_fd.c)
the length is not know before, so we get it from the header but we check it
anyway that it's within the valid range.

Still for TCP it you could read "garbage" pre-allocated memory but I
don't know how much it is a risk, it might be a good idea to zero it
post allocation (I mean pdu->sdata). Allocated at:

https://github.com/torvalds/linux/blob/master/net/9p/client.c#L236

quoted

Just a note on transports here, I totally had forgotten about trans_xen
when we discussed this earlier as it is fairly new, but it looks like it
sets the length in the fcall properly so it should work without any
change.

I however cannot test trans=xen, so if someone could either point me to
how to set that up (I couldn't find any decent documentation) or do some
very basic tests that would be great.

quoted

quoted

Signed-off-by: Tomas Bortoli <redacted>
Reported-by: syzbot+65c6b72f284a39d416b4@syzkaller.appspotmail.com

Looks good to me, as the rdma/virtio part come from my suggestion:
Signed-off-by: Dominique Martinet <redacted>

True

quoted

quoted

diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 3d414acb7015..002badbcc9c0 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c

@@ -319,7 +319,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;

(nitpick, I'd keep the empty line here. If you don't mind I'll add it
back in my tree; this doesn't warrant a v2)

Sure,

Tomas