net/memif: validate input from connecting peer

index 5d153c3a5a..89796d3ed5 100644
@@ -1595,25 +1595,18 @@ static int
 memif_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 		struct eth_queue_stats *qstats)
 {
-	struct pmd_internals *pmd = dev->data->dev_private;
 	struct memif_queue *mq;
-	int i;
-	uint8_t tmp, nq;
-
-	stats->ipackets = 0;
-	stats->ibytes = 0;
-	stats->opackets = 0;
-	stats->obytes = 0;
+	unsigned int i;
 
-	tmp = (pmd->role == MEMIF_ROLE_CLIENT) ? pmd->run.num_s2c_rings :
-	    pmd->run.num_c2s_rings;
-	nq = (tmp < RTE_ETHDEV_QUEUE_STAT_CNTRS) ? tmp :
-	    RTE_ETHDEV_QUEUE_STAT_CNTRS;
+	/*
+	 * Use the configured queue counts, not pmd->run, which memif_disconnect()
+	 * clears; otherwise all counters would read zero once the peer is gone.
+	 */
 
 	/* RX stats */
-	for (i = 0; i < nq; i++) {
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		mq = dev->data->rx_queues[i];
-		if (qstats != NULL) {
+		if (qstats != NULL && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
 			qstats->q_ipackets[i] = mq->n_pkts;
 			qstats->q_ibytes[i] = mq->n_bytes;
 		}

@@ -1622,15 +1615,10 @@ memif_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 		stats->ierrors += mq->n_err;
 	}
 
-	tmp = (pmd->role == MEMIF_ROLE_CLIENT) ? pmd->run.num_c2s_rings :
-	    pmd->run.num_s2c_rings;
-	nq = (tmp < RTE_ETHDEV_QUEUE_STAT_CNTRS) ? tmp :
-	    RTE_ETHDEV_QUEUE_STAT_CNTRS;
-
 	/* TX stats */
-	for (i = 0; i < nq; i++) {
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		mq = dev->data->tx_queues[i];
-		if (qstats != NULL) {
+		if (qstats != NULL && i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
 			qstats->q_opackets[i] = mq->n_pkts;
 			qstats->q_obytes[i] = mq->n_bytes;
 		}

@@ -1643,20 +1631,18 @@ memif_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats,
 static int
 memif_stats_reset(struct rte_eth_dev *dev)
 {
-	struct pmd_internals *pmd = dev->data->dev_private;
-	int i;
 	struct memif_queue *mq;
+	unsigned int i;
 
-	for (i = 0; i < pmd->run.num_c2s_rings; i++) {
-		mq = (pmd->role == MEMIF_ROLE_CLIENT) ? dev->data->tx_queues[i] :
-		    dev->data->rx_queues[i];
+	/* Same as memif_stats_get(), pmd->run is cleared on disconnect. */
+	for (i = 0; i < dev->data->nb_rx_queues; i++) {
+		mq = dev->data->rx_queues[i];
 		mq->n_pkts = 0;
 		mq->n_bytes = 0;
 		mq->n_err = 0;
 	}
-	for (i = 0; i < pmd->run.num_s2c_rings; i++) {
-		mq = (pmd->role == MEMIF_ROLE_CLIENT) ? dev->data->rx_queues[i] :
-		    dev->data->tx_queues[i];
+	for (i = 0; i < dev->data->nb_tx_queues; i++) {
+		mq = dev->data->tx_queues[i];
 		mq->n_pkts = 0;
 		mq->n_bytes = 0;
 		mq->n_err = 0;

439 lines not on this page · raw message

index 9c7a3a93f0..398a1f7baf 100644
@@ -98,6 +98,7 @@ struct pmd_internals {
 
 	struct memif_control_channel *cc;	/**< control channel */
 	rte_spinlock_t cc_lock;			/**< control channel lock */
+	RTE_ATOMIC(bool) bad_desc;		/**< peer supplied bad descriptor */
 
 	/* remote info */
 	char remote_name[RTE_DEV_NAME_MAX_LEN];		/**< remote app name */

index 898ad75fa6..8231a936e5 100644
@@ -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

@@ -658,17 +726,11 @@ memif_msg_receive(struct memif_control_channel *cc)
 		return -1;
 
 	size = recvmsg(rte_intr_fd_get(cc->intr_handle), &mh, 0);
-	if (size != sizeof(memif_msg_t)) {
-		MIF_LOG(DEBUG, "Invalid message size = %zd", size);
-		if (size > 0)
-			/* 0 means end-of-file, negative size means error,
-			 * don't send further disconnect message in such cases.
-			 */
-			memif_msg_enq_disconnect(cc, "Invalid message size", 0);
-		return -1;
-	}
-	MIF_LOG(DEBUG, "Received msg type: %u.", msg.type);
 
+	/*
+	 * Collect any passed fd first; a short message can still carry one,
+	 * and it has to be closed on every path out of this function.
+	 */
 	cmsg = CMSG_FIRSTHDR(&mh);
 	while (cmsg) {
 		if (cmsg->cmsg_level == SOL_SOCKET) {

@@ -680,10 +742,23 @@ memif_msg_receive(struct memif_control_channel *cc)
 		cmsg = CMSG_NXTHDR(&mh, cmsg);
 	}
 
+	if (size != sizeof(memif_msg_t)) {
+		MIF_LOG(DEBUG, "Invalid message size = %zd", size);
+		if (size > 0)
+			/* 0 means end-of-file, negative size means error,
+			 * don't send further disconnect message in such cases.
+			 */
+			memif_msg_enq_disconnect(cc, "Invalid message size", 0);
+		ret = -1;
+		goto exit;
+	}
+	MIF_LOG(DEBUG, "Received msg type: %u.", msg.type);
+
 	if (cc->dev == NULL && msg.type != MEMIF_MSG_TYPE_INIT) {
 		MIF_LOG(DEBUG, "Unexpected message.");
 		memif_msg_enq_disconnect(cc, "Unexpected message", 0);
-		return -1;
+		ret = -1;
+		goto exit;
 	}
 
 	/* get device from hash data */

@@ -736,7 +811,9 @@ memif_msg_receive(struct memif_control_channel *cc)
 			goto exit;
 		break;
 	case MEMIF_MSG_TYPE_ADD_REGION:
+		/* The handler owns the fd from here, on success and on error. */
 		ret = memif_msg_receive_add_region(cc->dev, &msg, afd);
+		afd = -1;
 		if (ret < 0)
 			goto exit;
 		ret = memif_msg_enq_ack(cc->dev);

@@ -744,7 +821,9 @@ memif_msg_receive(struct memif_control_channel *cc)
 			goto exit;
 		break;
 	case MEMIF_MSG_TYPE_ADD_RING:
+		/* The handler owns the fd from here, on success and on error. */
 		ret = memif_msg_receive_add_ring(cc->dev, &msg, afd);
+		afd = -1;
 		if (ret < 0)
 			goto exit;
 		ret = memif_msg_enq_ack(cc->dev);

@@ -774,6 +853,14 @@ memif_msg_receive(struct memif_control_channel *cc)
 	}
 
  exit:
+	/*
+	 * A peer can attach an fd to any message, but only add region and
+	 * add ring take one. Close the rest, otherwise a client could
+	 * exhaust the file descriptors of the server.
+	 */
+	if (afd >= 0)
+		close(afd);
+
 	return ret;
 }
 

index f7be4e4f4b..7dbc80d3b0 100644
@@ -711,7 +711,11 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	memif_ring_t *ring = memif_get_ring_from_queue(proc_private, mq);
 	uint16_t cur_slot, last_slot, n_slots, ring_size, mask, s0, head;
 	uint16_t n_rx_pkts = 0;
+	/* Buffer size advertised to the peer by the refill loop below. */
+	const uint16_t buf_size = rte_pktmbuf_data_room_size(mq->mempool) -
+		RTE_PKTMBUF_HEADROOM;
 	memif_desc_t *d0;
+	memif_desc_t desc;
 	struct rte_mbuf *mbuf, *mbuf_tail;
 	struct rte_mbuf *mbuf_head = NULL;
 	int ret;

@@ -763,14 +767,31 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			rte_prefetch0(&ring->desc[(cur_slot + 1) & mask]);
 
 		mbuf->port = mq->in_port;
-		rte_pktmbuf_data_len(mbuf) = d0->length;
-		rte_pktmbuf_pkt_len(mbuf) = rte_pktmbuf_data_len(mbuf);
+		desc = memif_desc_read(d0);
+
+		/* The peer only supplies the length here */
+		if (unlikely(desc.length > buf_size)) {
+			memif_desc_error(mq, &desc, MEMIF_DESC_STATUS_ERR_DATA_TOO_BIG);
+			/* Consume the slot before discarding */
+			cur_slot++;
+			n_slots--;
+			goto discard;
+		}
 
-		mq->n_bytes += rte_pktmbuf_data_len(mbuf);
+		rte_pktmbuf_data_len(mbuf) = desc.length;
+		rte_pktmbuf_pkt_len(mbuf) = desc.length;
+		if (mbuf != mbuf_head)
+			rte_pktmbuf_pkt_len(mbuf_head) += desc.length;
+
+		mq->n_bytes += desc.length;
 
 		cur_slot++;
 		n_slots--;
-		if (d0->flags & MEMIF_DESC_FLAG_NEXT) {
+		if (desc.flags & MEMIF_DESC_FLAG_NEXT) {
+			if (unlikely(n_slots == 0)) {
+				mq->n_err++;
+				goto discard;
+			}
 			s0 = cur_slot & mask;
 			d0 = &ring->desc[s0];
 			mbuf_tail = mbuf;

@@ -778,7 +799,8 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 			ret = memif_pktmbuf_chain(mbuf_head, mbuf_tail, mbuf);
 			if (unlikely(ret < 0)) {
 				MIF_LOG(ERR, "number-of-segments-overflow");
-				goto refill;
+				mq->n_err++;
+				goto discard;
 			}
 			goto next_slot;
 		}

@@ -788,6 +810,17 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	}
 
 	mq->last_tail = cur_slot;
+	goto refill;
+
+discard:
+	/*
+	 * The peer is buggy or hostile, remaining descriptors cannot be trusted.
+	 * Drop the partially built packet and the slots not yet consumed.
+	 */
+	rte_pktmbuf_free(mbuf_head);
+	while (n_slots--)
+		rte_pktmbuf_free_seg(mq->buffers[cur_slot++ & mask]);
+	mq->last_tail = cur_slot;
 
 /* Supply server with new buffers */
 refill:

@@ -820,8 +853,9 @@ eth_memif_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		d0->length = rte_pktmbuf_data_room_size(mq->mempool) -
 				RTE_PKTMBUF_HEADROOM;
 		d0->region = 1;
+		/* Use the constant, the peer can change d0->region at any time. */
 		d0->offset = rte_pktmbuf_mtod(mbuf, uint8_t *) -
-			(uint8_t *)proc_private->regions[d0->region]->addr;
+			(uint8_t *)proc_private->regions[1]->addr;
 	}
 no_free_mbufs:
 	/* The ring->head acts as a guard variable between Tx and Rx

@@ -1096,8 +1130,9 @@ memif_tx_one_zc(struct pmd_process_private *proc_private, struct memif_queue *mq
 	mq->n_bytes += rte_pktmbuf_data_len(mbuf);
 	/* FIXME: get region index */
 	d0->region = 1;
+	/* Use the constant, the peer can change d0->region at any time. */
 	d0->offset = rte_pktmbuf_mtod(mbuf, uint8_t *) -
-		(uint8_t *)proc_private->regions[d0->region]->addr;
+		(uint8_t *)proc_private->regions[1]->addr;
 	d0->flags = 0;
 
 	/* check if buffer is chained */