[RFC 3/6] net/memif: validate control channel requests

[Stephen Hemminger]

The server can not trust the connecting client, validate
connection requests before acting on them.

Check the file passed with region request to make sure that the
claimed region size is backed by the file. Mapping beyond the end
of a file succeeds and only faults on access, so a client that
overstates the size can otherwise fault the server with SIGBUS.
This also underpins the data path descriptor checks, which bound
each descriptor against the region size.

Warn when the region is not sealed against shrinking, but do not
reject it. Requiring a seal would disconnect conforming clients:
VPP's hugepage backed regions can not be sealed, and a memfd
created without MFD_ALLOW_SEALING can not be distinguished from
one whose owner chose not to seal.

For an add ring request: require rings to be added in order exactly
once so the ring counts can not exceed the number of configured
queues, bound the ring size to the advertised maximum, reject
unsupported private headers, and check that the referenced region
exists and that the ring plus its descriptor table fits inside that
region at a naturally aligned offset.

Bugzilla ID: 2011
Bugzilla ID: 2012
Bugzilla ID: 2013
Bugzilla ID: 2019
Fixes: 09c7e63a71f9 ("net/memif: introduce memory interface PMD")
Cc: stable at dpdk.org

Reported-by: Arthur Chan <arthur.chan at adalogics.com>
Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
---
 drivers/net/memif/memif_socket.c | 88 ++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index 649f8d0e61..71c9a21302 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -7,6 +7,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/socket.h>
+#include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <errno.h>
 
@@ -262,6 +263,8 @@ memif_msg_receive_add_region(struct rte_eth_dev *dev, memif_msg_t *msg,
 	struct pmd_process_private *proc_private = dev->process_private;
 	memif_msg_add_region_t *ar = &msg->add_region;
 	struct memif_region *r;
+	struct stat st;
+	int seals;
 
 	if (fd < 0) {
 		memif_msg_enq_disconnect(pmd->cc, "Missing region fd", 0);
@@ -272,13 +275,32 @@ memif_msg_receive_add_region(struct rte_eth_dev *dev, memif_msg_t *msg,
 			ar->index != proc_private->regions_num ||
 			proc_private->regions[ar->index] != NULL) {
 		memif_msg_enq_disconnect(pmd->cc, "Invalid region index", 0);
-		return -1;
+		goto error;
+	}
+
+	/* The client is not trusted to describe the region it shares. */
+	if (ar->size == 0 || fstat(fd, &st) < 0 || (uint64_t)st.st_size < ar->size) {
+		memif_msg_enq_disconnect(pmd->cc, "Invalid region size", 0);
+		goto error;
 	}
 
+	/*
+	 * Prefer regions sealed against shrinking so the peer can not truncate
+	 * the file after connect and fault the server. Sealing is not supported
+	 * on all fd types (for example hugetlbfs).
+	 */
+	seals = fcntl(fd, F_GET_SEALS);
+	if (seals < 0)
+		MIF_LOG(INFO, "Port %u region %u fd does not support sealing",
+			dev->data->port_id, ar->index);
+	else if ((seals & F_SEAL_SHRINK) == 0)
+		MIF_LOG(NOTICE, "Port %u region %u fd is not sealed against shrinking",
+			dev->data->port_id, ar->index);
+
 	r = rte_zmalloc("region", sizeof(struct memif_region), 0);
 	if (r == NULL) {
 		memif_msg_enq_disconnect(pmd->cc, "Failed to alloc memif region.", 0);
-		return -ENOMEM;
+		goto error;
 	}
 
 	r->fd = fd;
@@ -289,46 +311,92 @@ memif_msg_receive_add_region(struct rte_eth_dev *dev, memif_msg_t *msg,
 	proc_private->regions_num++;
 
 	return 0;
+error:
+	close(fd);
+	return -1;
 }
 
 static int
 memif_msg_receive_add_ring(struct rte_eth_dev *dev, memif_msg_t *msg, int fd)
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
+	struct pmd_process_private *proc_private = dev->process_private;
 	memif_msg_add_ring_t *ar = &msg->add_ring;
+	const struct memif_region *r;
 	struct memif_queue *mq;
+	uint64_t ring_size;
 
 	if (fd < 0) {
 		memif_msg_enq_disconnect(pmd->cc, "Missing interrupt fd", 0);
 		return -1;
 	}
 
-	/* check if we have enough queues */
+	/* rings must be added in order, exactly once */
 	if (ar->flags & MEMIF_MSG_ADD_RING_FLAG_C2S) {
-		if (ar->index >= pmd->cfg.num_c2s_rings) {
+		if (ar->index >= pmd->cfg.num_c2s_rings ||
+				ar->index != pmd->run.num_c2s_rings) {
 			memif_msg_enq_disconnect(pmd->cc, "Invalid ring index", 0);
-			return -1;
+			goto error;
 		}
-		pmd->run.num_c2s_rings++;
 	} else {
-		if (ar->index >= pmd->cfg.num_s2c_rings) {
+		if (ar->index >= pmd->cfg.num_s2c_rings ||
+				ar->index != pmd->run.num_s2c_rings) {
 			memif_msg_enq_disconnect(pmd->cc, "Invalid ring index", 0);
-			return -1;
+			goto error;
 		}
-		pmd->run.num_s2c_rings++;
+	}
+
+	if (ar->log2_ring_size > ETH_MEMIF_MAX_LOG2_RING_SIZE) {
+		memif_msg_enq_disconnect(pmd->cc, "Invalid ring size", 0);
+		goto error;
+	}
+
+	/* private headers are not supported */
+	if (ar->private_hdr_size != 0) {
+		memif_msg_enq_disconnect(pmd->cc, "Unsupported private header", 0);
+		goto error;
+	}
+
+	if (ar->region >= proc_private->regions_num ||
+			proc_private->regions[ar->region] == NULL) {
+		memif_msg_enq_disconnect(pmd->cc, "Invalid region index", 0);
+		goto error;
+	}
+
+	/*
+	 * The ring and its descriptors must lie inside the region.
+	 * Require natural alignment of the ring for atomic load/store.
+	 * Existing DPDK and VPP put it on cache line boundary.
+	 */
+	r = proc_private->regions[ar->region];
+	ring_size = sizeof(memif_ring_t) +
+		sizeof(memif_desc_t) * ((uint64_t)1 << ar->log2_ring_size);
+	if ((ar->offset & (sizeof(uint64_t) - 1)) != 0 ||
+			ar->offset + ring_size > r->region_size) {
+		memif_msg_enq_disconnect(pmd->cc, "Invalid ring offset", 0);
+		goto error;
 	}
 
 	mq = (ar->flags & MEMIF_MSG_ADD_RING_FLAG_C2S) ?
 	    dev->data->rx_queues[ar->index] : dev->data->tx_queues[ar->index];
 
+	/* Takes ownership of the fd, so nothing to close after this point. */
 	if (rte_intr_fd_set(mq->intr_handle, fd))
-		return -1;
+		goto error;
+
+	if (ar->flags & MEMIF_MSG_ADD_RING_FLAG_C2S)
+		pmd->run.num_c2s_rings++;
+	else
+		pmd->run.num_s2c_rings++;
 
 	mq->log2_ring_size = ar->log2_ring_size;
 	mq->region = ar->region;
 	mq->ring_offset = ar->offset;
 
 	return 0;
+error:
+	close(fd);
+	return -1;
 }
 
 static int
-- 
2.53.0