p9_parse_header() validate PDU length

PATCH In review Tomas Bortoli 2 participants 6 messages Jul 12, 2018 – Jul 18, 2018

Subsystems (3)

Networking Network drivers 9P file system

lore

Share & tools

lore

Tomas Bortoli

author

2018-07-12 11:02 UTC #0 lore raw

PATCH 1/1 p9_parse_header() validate PDU length

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.

Signed-off-by Tomas Bortoli <[email protected]>

Reported-by [email protected]

---
 net/9p/client.c       | 26 ++++++++++++++++----------
 net/9p/trans_fd.c     | 17 ++++++-----------
 net/9p/trans_rdma.c   |  2 +-
 net/9p/trans_virtio.c |  6 ++++--
 4 files changed, 27 insertions(+), 24 deletions(-)

net/9p/client.c +16 ~~-10~~

index 18c5271910dc..119de44f49e9 100644
@@ -477,20 +477,11 @@ p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag,
 	int err;
 
 	pdu->offset = 0;
-	if (pdu->size == 0)
-		pdu->size = 7;
 
 	err = p9pdu_readf(pdu, 0, "dbw", &r_size, &r_type, &r_tag);
 	if (err)
 		goto rewind_and_exit;
 
-	pdu->size = r_size;
-	pdu->id = r_type;
-	pdu->tag = r_tag;
-
-	p9_debug(P9_DEBUG_9P, "<<< size=%d type: %d tag: %d\n",
-		 pdu->size, pdu->id, pdu->tag);
-
 	if (type)
 		*type = r_type;
 	if (tag)

@@ -498,6 +489,21 @@ p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag,
 	if (size)
 		*size = r_size;
 
+	if (pdu->size != r_size) {
+		err = -EINVAL;
+		goto rewind_and_exit;
+	}
+	if (pdu->size >= pdu->capacity || pdu->size < 7) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "requested packet size too big or too small: %d\n",
+			 pdu->size);
+		return -EIO;
+	}
+	pdu->id = r_type;
+	pdu->tag = r_tag;
+
+	p9_debug(P9_DEBUG_9P, "<<< size=%d type: %d tag: %d\n",
+		 pdu->size, pdu->id, pdu->tag);
 
 rewind_and_exit:
 	if (rewind)

@@ -1575,7 +1581,7 @@ p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
 		int count = iov_iter_count(to);
 		int rsize, non_zc = 0;
 		char *dataptr;
-			
+
 		rsize = fid->iounit;
 		if (!rsize || rsize > clnt->msize-P9_IOHDRSZ)
 			rsize = clnt->msize - P9_IOHDRSZ;

net/9p/trans_fd.c +6 ~~-11~~

index 588bf88c3305..bf459ee0feab 100644
@@ -323,22 +323,16 @@ static void p9_read_work(struct work_struct *work)
 	/* header read in */
 	if ((!m->req) && (m->rc.offset == m->rc.capacity)) {
 		p9_debug(P9_DEBUG_TRANS, "got new header\n");
-
-		err = p9_parse_header(&m->rc, NULL, NULL, NULL, 0);
+		/* Header size */
+		m->rc.size = 7;
+		m->rc.capacity = m->client->msize;
+		err = p9_parse_header(&m->rc, &m->rc.size, NULL, NULL, 0);
 		if (err) {
 			p9_debug(P9_DEBUG_ERROR,
 				 "error parsing header: %d\n", err);
 			goto error;
 		}
 
-		if (m->rc.size >= m->client->msize) {
-			p9_debug(P9_DEBUG_ERROR,
-				 "requested packet size too big: %d\n",
-				 m->rc.size);
-			err = -EIO;
-			goto error;
-		}
-
 		p9_debug(P9_DEBUG_TRANS,
 			 "mux %p pkt: size: %d bytes tag: %d\n",
 			 m, m->rc.size, m->rc.tag);

@@ -360,7 +354,7 @@ static void p9_read_work(struct work_struct *work)
 			goto error;
 		}
 		m->rc.sdata = (char *)m->req->rc + sizeof(struct p9_fcall);
-		memcpy(m->rc.sdata, m->tmp_buf, m->rc.capacity);
+		memcpy(m->rc.sdata, m->tmp_buf, 7);
 		m->rc.capacity = m->rc.size;
 	}

@@ -369,6 +363,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;

net/9p/trans_rdma.c +1 -1

index 3d414acb7015..002badbcc9c0 100644
@@ -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;
 	err = p9_parse_header(c->rc, NULL, NULL, &tag, 1);
 	if (err)
 		goto err_out;

net/9p/trans_virtio.c +4 -2

index 05006cbb3361..6d515f7ebfaf 100644
@@ -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);
+		}
 	}
 }

@@ -446,7 +448,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
 		out += pack_sg_list_p(chan->sg, out, VIRTQUEUE_NUM,
 				      out_pages, out_nr_pages, offs, outlen);
 	}
-		
+
 	/*
 	 * Take care of in data
 	 * For example TREAD have 11.

Signature · 1 line

2.11.0

Dominique Martinet

maintainer

2018-07-12 11:43 UTC #1 lore raw

Tomas Bortoli wrote on Thu, Jul 12, 2018:

Quoted 7 lines

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.

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 2 lines

Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]

Looks good to me, as the rdma/virtio part come from my suggestion:

Signed-off-by Dominique Martinet <[email protected]>

net/9p/trans_rdma.c recv_done(struct ib_cq *cq, struct ib_wc *wc) · line 322

Quoted 10 lines

net/9p/trans_rdma.c +1 -1

index 3d414acb7015..002badbcc9c0 100644
@@ -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)

Signature · 1 line

Dominique Martinet

Tomas Bortoli

author

2018-07-12 16:19 UTC #2 lore raw

+ Cc: Andrew Morton <[email protected]>

Quoted 8 lines · Dominique Martinet

Tomas Bortoli wrote on Thu, Jul 12, 2018:
Quoted 7 lines
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 5 lines

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.

Quoted 8 lines

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 2 lines
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
Looks good to me, as the rdma/virtio part come from my suggestion:
Signed-off-by Dominique Martinet <[email protected]>

True

Quoted 7 lines

net/9p/trans_rdma.c +0 -0

index 3d414acb7015..002badbcc9c0 100644
@@ -319,7 +319,7 @@ recv_done(struct ib_cq *cq, struct ib_wc *wc)

Quoted 7 lines

net/9p/trans_rdma.c recv_done(struct ib_cq *cq, struct ib_wc *wc) · line 322
Quoted 4 lines
net/9p/trans_rdma.c +1 -1
 	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

Dominique Martinet

maintainer

2018-07-18 05:09 UTC #3 lore raw

Tomas Bortoli wrote on Thu, Jul 12, 2018:

Quoted 52 lines

+ Cc: Andrew Morton <[email protected]>
Quoted 8 lines · Dominique Martinet
Tomas Bortoli wrote on Thu, Jul 12, 2018:
Quoted 7 lines
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 5 lines
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.
Quoted 8 lines
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 2 lines
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
Looks good to me, as the rdma/virtio part come from my suggestion:
Signed-off-by Dominique Martinet <[email protected]>
True
Quoted 7 lines
Quoted 7 lines
net/9p/trans_rdma.c +0 -0
index 3d414acb7015..002badbcc9c0 100644
@@ -319,7 +319,7 @@ recv_done(struct ib_cq *cq, struct ib_wc *wc)
 
Quoted 7 lines
net/9p/trans_rdma.c recv_done(struct ib_cq *cq, struct ib_wc *wc) · line 322
Quoted 4 lines
net/9p/trans_rdma.c +1 -1
 	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

Dominique Martinet

maintainer

2018-07-18 05:13 UTC #4 lore raw

In reply to Tomas Bortoli

Tomas Bortoli wrote on Thu, Jul 12, 2018:

Quoted 16 lines

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.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
---
[..]
net/9p/client.c +0 -0
@@ -498,6 +489,21 @@ p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag,
 	if (size)
 		*size = r_size;
 

net/9p/client.c p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag, · line 501

Quoted 10 lines

net/9p/client.c +10 -0

+	if (pdu->size != r_size) {
+		err = -EINVAL;
+		goto rewind_and_exit;
+	}
+	if (pdu->size >= pdu->capacity || pdu->size < 7) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "requested packet size too big or too small: %d\n",
+			 pdu->size);
+		return -EIO;
+	}

Actually, I've been bad advice - this breaks on virtio with zc packets - a read or ls in a big directory fails with this in dmesg

[ 1006.853775] 9pnet: -- p9_parse_header (17123): requested packet size too big or too small: 4306
[ 1006.853780] 9pnet: -- p9_check_zc_errors (17123): couldn't parse header -5

I haven't given this any thought yet, but dropping the patch for now

Signature · 1 line

Dominique

Tomas Bortoli

author

2018-07-18 08:39 UTC #5 lore raw

Quoted 29 lines · Dominique Martinet

Tomas Bortoli wrote on Thu, Jul 12, 2018:
Quoted 16 lines
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.
Signed-off-by Tomas Bortoli <[email protected]>
Reported-by [email protected]
---
[..]
net/9p/client.c +0 -0
@@ -498,6 +489,21 @@ p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag,
 	if (size)
 		*size = r_size;
 
net/9p/client.c p9_parse_header(struct p9_fcall *pdu, int32_t *size, int8_t *type, int16_t *tag, · line 501
Quoted 10 lines
net/9p/client.c +10 -0
+	if (pdu->size != r_size) {
+		err = -EINVAL;
+		goto rewind_and_exit;
+	}
+	if (pdu->size >= pdu->capacity || pdu->size < 7) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "requested packet size too big or too small: %d\n",
+			 pdu->size);
+		return -EIO;
+	}
Actually, I've been bad advice - this breaks on virtio with zc packets - a read or ls in a big directory fails with this in dmesg

Quoted 5 lines

[ 1006.853775] 9pnet: -- p9_parse_header (17123): requested packet size too big or too small: 4306
[ 1006.853780] 9pnet: -- p9_check_zc_errors (17123): couldn't parse header -5

I haven't given this any thought yet, but dropping the patch for now

It seems that pdu->capacity is set to less than 4306 at that point, we can just increase it.