All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: dev@dpdk.org
Cc: maxime.coquelin@redhat.com, chenbo.xia@intel.com
Subject: [PATCH 2/2] vhost: validate fds attached to messages
Date: Mon, 25 Apr 2022 14:54:30 +0200	[thread overview]
Message-ID: <20220425125431.26464-2-david.marchand@redhat.com> (raw)
In-Reply-To: <20220425125431.26464-1-david.marchand@redhat.com>

Some message handlers do not expect any file descriptor attached as
ancillary data.
Provide a common way to enforce this by adding a accepts_fd boolean in
the message handler structure. When a message handler sets accepts_fd to
true, it is responsible for calling validate_msg_fds with a right
expected file descriptor count.
This will avoid leaking some file descriptor by mistake when adding
support for new vhost user message types.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost_user.c | 145 ++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 102 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 17cfeafa16..850848c269 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -60,6 +60,7 @@ typedef struct vhost_message_handler {
 	const char *description;
 	int (*callback)(struct virtio_net **pdev, struct vhu_msg_context *ctx,
 		int main_fd);
+	bool accepts_fd;
 } vhost_message_handler_t;
 static vhost_message_handler_t vhost_message_handlers[];
 
@@ -262,28 +263,20 @@ vhost_user_notify_queue_state(struct virtio_net *dev, uint16_t index,
  * the device hasn't been initialised.
  */
 static int
-vhost_user_set_owner(struct virtio_net **pdev,
-			struct vhu_msg_context *ctx,
+vhost_user_set_owner(struct virtio_net **pdev __rte_unused,
+			struct vhu_msg_context *ctx __rte_unused,
 			int main_fd __rte_unused)
 {
-	struct virtio_net *dev = *pdev;
-
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	return RTE_VHOST_MSG_RESULT_OK;
 }
 
 static int
 vhost_user_reset_owner(struct virtio_net **pdev,
-			struct vhu_msg_context *ctx,
+			struct vhu_msg_context *ctx __rte_unused,
 			int main_fd __rte_unused)
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	vhost_destroy_device_notify(dev);
 
 	cleanup_device(dev, 0);
@@ -302,9 +295,6 @@ vhost_user_get_features(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	uint64_t features = 0;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	rte_vhost_driver_get_features(dev->ifname, &features);
 
 	ctx->msg.payload.u64 = features;
@@ -325,9 +315,6 @@ vhost_user_get_queue_num(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	uint32_t queue_num = 0;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	rte_vhost_driver_get_queue_num(dev->ifname, &queue_num);
 
 	ctx->msg.payload.u64 = (uint64_t)queue_num;
@@ -350,9 +337,6 @@ vhost_user_set_features(struct virtio_net **pdev,
 	uint64_t vhost_features = 0;
 	struct rte_vdpa_device *vdpa_dev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	rte_vhost_driver_get_features(dev->ifname, &vhost_features);
 	if (features & ~vhost_features) {
 		VHOST_LOG_CONFIG(ERR, "(%s) received invalid negotiated features.\n",
@@ -438,9 +422,6 @@ vhost_user_set_vring_num(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index];
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (ctx->msg.payload.state.num > 32768) {
 		VHOST_LOG_CONFIG(ERR, "(%s) invalid virtqueue size %u\n",
 				dev->ifname, ctx->msg.payload.state.num);
@@ -882,9 +863,6 @@ vhost_user_set_vring_addr(struct virtio_net **pdev,
 	struct vhost_vring_addr *addr = &ctx->msg.payload.addr;
 	bool access_ok;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (dev->mem == NULL)
 		return RTE_VHOST_MSG_RESULT_ERR;
 
@@ -926,9 +904,6 @@ vhost_user_set_vring_base(struct virtio_net **pdev,
 	struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index];
 	uint64_t val = ctx->msg.payload.state.num;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (vq_is_packed(dev)) {
 		/*
 		 * Bit[0:14]: avail index
@@ -1574,9 +1549,6 @@ vhost_user_get_inflight_fd(struct virtio_net **pdev,
 	int numa_node = SOCKET_ID_ANY;
 	void *addr;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (ctx->msg.size != sizeof(ctx->msg.payload.inflight)) {
 		VHOST_LOG_CONFIG(ERR, "(%s) invalid get_inflight_fd message size is %d\n",
 			dev->ifname, ctx->msg.size);
@@ -2097,9 +2069,6 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index];
 	uint64_t val;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	/* We have to stop the queue (virtio) if it is running. */
 	vhost_destroy_device_notify(dev);
 
@@ -2176,9 +2145,6 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 	bool enable = !!ctx->msg.payload.state.num;
 	int index = (int)ctx->msg.payload.state.index;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	VHOST_LOG_CONFIG(INFO, "(%s) set queue enable: %d to qp idx: %d\n",
 			dev->ifname, enable, index);
 
@@ -2204,9 +2170,6 @@ vhost_user_get_protocol_features(struct virtio_net **pdev,
 	struct virtio_net *dev = *pdev;
 	uint64_t features, protocol_features;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	rte_vhost_driver_get_features(dev->ifname, &features);
 	rte_vhost_driver_get_protocol_features(dev->ifname, &protocol_features);
 
@@ -2226,9 +2189,6 @@ vhost_user_set_protocol_features(struct virtio_net **pdev,
 	uint64_t protocol_features = ctx->msg.payload.u64;
 	uint64_t slave_protocol_features = 0;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	rte_vhost_driver_get_protocol_features(dev->ifname,
 			&slave_protocol_features);
 	if (protocol_features & ~slave_protocol_features) {
@@ -2368,9 +2328,6 @@ vhost_user_send_rarp(struct virtio_net **pdev,
 	uint8_t *mac = (uint8_t *)&ctx->msg.payload.u64;
 	struct rte_vdpa_device *vdpa_dev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	VHOST_LOG_CONFIG(DEBUG, "(%s) MAC: " RTE_ETHER_ADDR_PRT_FMT "\n",
 		dev->ifname, mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
 	memcpy(dev->mac.addr_bytes, mac, 6);
@@ -2397,9 +2354,6 @@ vhost_user_net_set_mtu(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (ctx->msg.payload.u64 < VIRTIO_MIN_MTU ||
 			ctx->msg.payload.u64 > VIRTIO_MAX_MTU) {
 		VHOST_LOG_CONFIG(ERR, "(%s) invalid MTU size (%"PRIu64")\n",
@@ -2523,9 +2477,6 @@ vhost_user_iotlb_msg(struct virtio_net **pdev,
 	uint16_t i;
 	uint64_t vva, len;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	switch (imsg->type) {
 	case VHOST_IOTLB_UPDATE:
 		len = imsg->size;
@@ -2584,9 +2535,6 @@ vhost_user_set_postcopy_advise(struct virtio_net **pdev,
 #ifdef RTE_LIBRTE_VHOST_POSTCOPY
 	struct uffdio_api api_struct;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	dev->postcopy_ufd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
 
 	if (dev->postcopy_ufd == -1) {
@@ -2622,9 +2570,6 @@ vhost_user_set_postcopy_listen(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	if (dev->mem && dev->mem->nregions) {
 		VHOST_LOG_CONFIG(ERR, "(%s) regions already registered at postcopy-listen\n",
 				dev->ifname);
@@ -2642,9 +2587,6 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	dev->postcopy_listening = 0;
 	if (dev->postcopy_ufd >= 0) {
 		close(dev->postcopy_ufd);
@@ -2665,9 +2607,6 @@ vhost_user_get_status(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	ctx->msg.payload.u64 = dev->status;
 	ctx->msg.size = sizeof(ctx->msg.payload.u64);
 	ctx->fd_num = 0;
@@ -2682,9 +2621,6 @@ vhost_user_set_status(struct virtio_net **pdev,
 {
 	struct virtio_net *dev = *pdev;
 
-	if (validate_msg_fds(dev, ctx, 0) != 0)
-		return RTE_VHOST_MSG_RESULT_ERR;
-
 	/* As per Virtio specification, the device status is 8bits long */
 	if (ctx->msg.payload.u64 > UINT8_MAX) {
 		VHOST_LOG_CONFIG(ERR, "(%s) invalid VHOST_USER_SET_STATUS payload 0x%" PRIx64 "\n",
@@ -2727,39 +2663,39 @@ vhost_user_set_status(struct virtio_net **pdev,
 }
 
 #define VHOST_MESSAGE_HANDLERS \
-VHOST_MESSAGE_HANDLER(VHOST_USER_NONE, NULL) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_FEATURES, vhost_user_get_features) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_FEATURES, vhost_user_set_features) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_OWNER, vhost_user_set_owner) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ADDR, vhost_user_set_vring_addr) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_BASE, vhost_user_set_vring_base) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_VRING_BASE, vhost_user_get_vring_base) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_KICK, vhost_user_set_vring_kick) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_CALL, vhost_user_set_vring_call) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ERR, vhost_user_set_vring_err) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_PROTOCOL_FEATURES, vhost_user_get_protocol_features) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_PROTOCOL_FEATURES, vhost_user_set_protocol_features) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_QUEUE_NUM, vhost_user_get_queue_num) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ENABLE, vhost_user_set_vring_enable) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SEND_RARP, vhost_user_send_rarp) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_NET_SET_MTU, vhost_user_net_set_mtu) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_SLAVE_REQ_FD, vhost_user_set_req_fd) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_IOTLB_MSG, vhost_user_iotlb_msg) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_ADVISE, vhost_user_set_postcopy_advise) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_LISTEN, vhost_user_set_postcopy_listen) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_END, vhost_user_postcopy_end) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_INFLIGHT_FD, vhost_user_get_inflight_fd) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_INFLIGHT_FD, vhost_user_set_inflight_fd) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_SET_STATUS, vhost_user_set_status) \
-VHOST_MESSAGE_HANDLER(VHOST_USER_GET_STATUS, vhost_user_get_status)
-
-#define VHOST_MESSAGE_HANDLER(id, handler) \
-	[id] = { #id, handler },
+VHOST_MESSAGE_HANDLER(VHOST_USER_NONE, NULL, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_FEATURES, vhost_user_get_features, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_FEATURES, vhost_user_set_features, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_OWNER, vhost_user_set_owner, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_RESET_OWNER, vhost_user_reset_owner, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_MEM_TABLE, vhost_user_set_mem_table, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_BASE, vhost_user_set_log_base, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_LOG_FD, vhost_user_set_log_fd, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_NUM, vhost_user_set_vring_num, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ADDR, vhost_user_set_vring_addr, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_BASE, vhost_user_set_vring_base, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_VRING_BASE, vhost_user_get_vring_base, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_KICK, vhost_user_set_vring_kick, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_CALL, vhost_user_set_vring_call, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ERR, vhost_user_set_vring_err, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_PROTOCOL_FEATURES, vhost_user_get_protocol_features, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_PROTOCOL_FEATURES, vhost_user_set_protocol_features, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_QUEUE_NUM, vhost_user_get_queue_num, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_VRING_ENABLE, vhost_user_set_vring_enable, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SEND_RARP, vhost_user_send_rarp, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_NET_SET_MTU, vhost_user_net_set_mtu, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_SLAVE_REQ_FD, vhost_user_set_req_fd, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_IOTLB_MSG, vhost_user_iotlb_msg, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_ADVISE, vhost_user_set_postcopy_advise, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_LISTEN, vhost_user_set_postcopy_listen, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_POSTCOPY_END, vhost_user_postcopy_end, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_INFLIGHT_FD, vhost_user_get_inflight_fd, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_INFLIGHT_FD, vhost_user_set_inflight_fd, true) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_SET_STATUS, vhost_user_set_status, false) \
+VHOST_MESSAGE_HANDLER(VHOST_USER_GET_STATUS, vhost_user_get_status, false)
+
+#define VHOST_MESSAGE_HANDLER(id, handler, accepts_fd) \
+	[id] = { #id, handler, accepts_fd },
 static vhost_message_handler_t vhost_message_handlers[] = {
 	VHOST_MESSAGE_HANDLERS
 };
@@ -3030,7 +2966,12 @@ vhost_user_msg_handler(int vid, int fd)
 	if (msg_handler == NULL || msg_handler->callback == NULL)
 		goto skip_to_post_handle;
 
-	ret = msg_handler->callback(&dev, &ctx, fd);
+	if (!msg_handler->accepts_fd && validate_msg_fds(dev, &ctx, 0) != 0) {
+		ret = RTE_VHOST_MSG_RESULT_ERR;
+	} else {
+		ret = msg_handler->callback(&dev, &ctx, fd);
+	}
+
 	switch (ret) {
 	case RTE_VHOST_MSG_RESULT_ERR:
 		VHOST_LOG_CONFIG(ERR, "(%s) processing %s failed.\n",
-- 
2.23.0


  reply	other threads:[~2022-04-25 12:54 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-25 12:54 [PATCH 1/2] vhost: remove unneeded max enums David Marchand
2022-04-25 12:54 ` David Marchand [this message]
2022-05-05 14:31   ` [PATCH 2/2] vhost: validate fds attached to messages Maxime Coquelin
2022-05-05 19:58   ` Maxime Coquelin
2022-05-05 14:22 ` [PATCH 1/2] vhost: remove unneeded max enums Maxime Coquelin
2022-05-05 19:57 ` Maxime Coquelin
2022-05-06  7:12   ` David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220425125431.26464-2-david.marchand@redhat.com \
    --to=david.marchand@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.