[PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[Andres Beltran]

From: Andres Beltran (Microsoft) <lkmlabelt@gmail.com>

Currently, VMbus drivers use pointers into guest memory as request IDs
for interactions with Hyper-V. To be more robust in the face of errors
or malicious behavior from a compromised Hyper-V, avoid exposing
guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
bad request ID that is then treated as the address of a guest data
structure with no validation. Instead, encapsulate these memory
addresses and provide small integers as request IDs.

The first patch creates the definitions for the data structure, provides
helper methods to generate new IDs and retrieve data, and
allocates/frees the memory needed for vmbus_requestor.

The second and third patches make use of vmbus_requestor to send request
IDs to Hyper-V in storvsc and netvsc respectively.

Thanks.
Andres Beltran

Cc: linux-scsi@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>

Andres Beltran (3):
  Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
    hardening
  scsi: storvsc: Use vmbus_requestor to generate transaction ids for
    VMBus hardening
  hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
    hardening

 drivers/hv/channel.c              | 150 ++++++++++++++++++++++++++++++
 drivers/net/hyperv/hyperv_net.h   |  10 ++
 drivers/net/hyperv/netvsc.c       |  56 +++++++++--
 drivers/net/hyperv/rndis_filter.c |   1 +
 drivers/scsi/storvsc_drv.c        |  62 ++++++++++--
 include/linux/hyperv.h            |  22 +++++
 6 files changed, 283 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

[Andres Beltran]

Currently, VMbus drivers use pointers into guest memory as request IDs
for interactions with Hyper-V. To be more robust in the face of errors
or malicious behavior from a compromised Hyper-V, avoid exposing
guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
bad request ID that is then treated as the address of a guest data
structure with no validation. Instead, encapsulate these memory
addresses and provide small integers as request IDs.

Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
---
 drivers/hv/channel.c   | 149 +++++++++++++++++++++++++++++++++++++++++
 include/linux/hyperv.h |  21 ++++++
 2 files changed, 170 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 3ebda7707e46..2ea1bfecbfda 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -112,6 +112,70 @@ int vmbus_alloc_ring(struct vmbus_channel *newchannel,
 }
 EXPORT_SYMBOL_GPL(vmbus_alloc_ring);
 
+/**
+ * request_arr_init - Allocates memory for the requestor array. Each slot
+ * keeps track of the next available slot in the array. Initially, each
+ * slot points to the next one (as in a Linked List). The last slot
+ * does not point to anything, so its value is U64_MAX by default.
+ * @size The size of the array
+ */
+static u64 *request_arr_init(u32 size)
+{
+	int i;
+	u64 *req_arr;
+
+	req_arr = kcalloc(size, sizeof(u64), GFP_KERNEL);
+	if (!req_arr)
+		return NULL;
+
+	for (i = 0; i < size - 1; i++)
+		req_arr[i] = i + 1;
+
+	/* Last slot (no more available slots) */
+	req_arr[i] = U64_MAX;
+
+	return req_arr;
+}
+
+/*
+ * vmbus_alloc_requestor - Initializes @rqstor's fields.
+ * Slot at index 0 is the first free slot.
+ * @size: Size of the requestor array
+ */
+static int vmbus_alloc_requestor(struct vmbus_requestor *rqstor, u32 size)
+{
+	u64 *rqst_arr;
+	unsigned long *bitmap;
+
+	rqst_arr = request_arr_init(size);
+	if (!rqst_arr)
+		return -ENOMEM;
+
+	bitmap = bitmap_zalloc(size, GFP_KERNEL);
+	if (!bitmap) {
+		kfree(rqst_arr);
+		return -ENOMEM;
+	}
+
+	rqstor->req_arr = rqst_arr;
+	rqstor->req_bitmap = bitmap;
+	rqstor->size = size;
+	rqstor->next_request_id = 0;
+	spin_lock_init(&rqstor->req_lock);
+
+	return 0;
+}
+
+/*
+ * vmbus_free_requestor - Frees memory allocated for @rqstor
+ * @rqstor: Pointer to the requestor struct
+ */
+static void vmbus_free_requestor(struct vmbus_requestor *rqstor)
+{
+	kfree(rqstor->req_arr);
+	bitmap_free(rqstor->req_bitmap);
+}
+
 static int __vmbus_open(struct vmbus_channel *newchannel,
 		       void *userdata, u32 userdatalen,
 		       void (*onchannelcallback)(void *context), void *context)
@@ -122,6 +186,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 	u32 send_pages, recv_pages;
 	unsigned long flags;
 	int err;
+	int rqstor;
 
 	if (userdatalen > MAX_USER_DEFINED_BYTES)
 		return -EINVAL;
@@ -132,6 +197,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 	if (newchannel->state != CHANNEL_OPEN_STATE)
 		return -EINVAL;
 
+	/* Create and init requestor */
+	if (newchannel->rqstor_size) {
+		rqstor = vmbus_alloc_requestor(&newchannel->requestor,
+					       newchannel->rqstor_size);
+		if (rqstor)
+			return -ENOMEM;
+	}
+
 	newchannel->state = CHANNEL_OPENING_STATE;
 	newchannel->onchannel_callback = onchannelcallback;
 	newchannel->channel_callback_context = context;
@@ -228,6 +301,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 error_clean_ring:
 	hv_ringbuffer_cleanup(&newchannel->outbound);
 	hv_ringbuffer_cleanup(&newchannel->inbound);
+	vmbus_free_requestor(&newchannel->requestor);
 	newchannel->state = CHANNEL_OPEN_STATE;
 	return err;
 }
@@ -703,6 +777,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
 		channel->ringbuffer_gpadlhandle = 0;
 	}
 
+	if (!ret)
+		vmbus_free_requestor(&channel->requestor);
+
 	return ret;
 }
 
@@ -937,3 +1014,75 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
 				  buffer_actual_len, requestid, true);
 }
 EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
+
+/*
+ * vmbus_next_request_id - Returns a new request id. It is also
+ * the index at which the guest memory address is stored.
+ * Uses a spin lock to avoid race conditions.
+ * @rqstor: Pointer to the requestor struct
+ * @rqst_add: Guest memory address to be stored in the array
+ */
+u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
+{
+	unsigned long flags;
+	u64 current_id;
+
+	spin_lock_irqsave(&rqstor->req_lock, flags);
+	current_id = rqstor->next_request_id;
+
+	/* Requestor array is full */
+	if (current_id >= rqstor->size) {
+		current_id = VMBUS_RQST_ERROR;
+		goto exit;
+	}
+
+	rqstor->next_request_id = rqstor->req_arr[current_id];
+	rqstor->req_arr[current_id] = rqst_addr;
+
+	/* The already held spin lock provides atomicity */
+	bitmap_set(rqstor->req_bitmap, current_id, 1);
+
+exit:
+	spin_unlock_irqrestore(&rqstor->req_lock, flags);
+	return current_id;
+}
+EXPORT_SYMBOL_GPL(vmbus_next_request_id);
+
+/*
+ * vmbus_request_addr - Returns the memory address stored at @trans_id
+ * in @rqstor. Uses a spin lock to avoid race conditions.
+ * @rqstor: Pointer to the requestor struct
+ * @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
+ * next request id.
+ */
+u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
+{
+	unsigned long flags;
+	u64 req_addr;
+
+	spin_lock_irqsave(&rqstor->req_lock, flags);
+
+	/* Invalid trans_id */
+	if (trans_id >= rqstor->size) {
+		req_addr = VMBUS_RQST_ERROR;
+		goto exit;
+	}
+
+	/* Invalid trans_id: empty slot */
+	if (!test_bit(trans_id, rqstor->req_bitmap)) {
+		req_addr = VMBUS_RQST_ERROR;
+		goto exit;
+	}
+
+	req_addr = rqstor->req_arr[trans_id];
+	rqstor->req_arr[trans_id] = rqstor->next_request_id;
+	rqstor->next_request_id = trans_id;
+
+	/* The already held spin lock provides atomicity */
+	bitmap_clear(rqstor->req_bitmap, trans_id, 1);
+
+exit:
+	spin_unlock_irqrestore(&rqstor->req_lock, flags);
+	return req_addr;
+}
+EXPORT_SYMBOL_GPL(vmbus_request_addr);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 38100e80360a..c509d20ab7db 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -716,6 +716,21 @@ enum vmbus_device_type {
 	HV_UNKNOWN,
 };
 
+/*
+ * Provides request ids for VMBus. Encapsulates guest memory
+ * addresses and stores the next available slot in req_arr
+ * to generate new ids in constant time.
+ */
+struct vmbus_requestor {
+	u64 *req_arr;
+	unsigned long *req_bitmap; /* is a given slot available? */
+	u32 size;
+	u64 next_request_id;
+	spinlock_t req_lock; /* provides atomicity */
+};
+
+#define VMBUS_RQST_ERROR U64_MAX
+
 struct vmbus_device {
 	u16  dev_type;
 	guid_t guid;
@@ -940,8 +955,14 @@ struct vmbus_channel {
 	u32 fuzz_testing_interrupt_delay;
 	u32 fuzz_testing_message_delay;
 
+	/* request/transaction ids for VMBus */
+	struct vmbus_requestor requestor;
+	u32 rqstor_size;
 };
 
+u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr);
+u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id);
+
 static inline bool is_hvsock_channel(const struct vmbus_channel *c)
 {
 	return !!(c->offermsg.offer.chn_flags &
-- 
2.25.1

[PATCH 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[Andres Beltran]

Currently, pointers to guest memory are passed to Hyper-V as
transaction IDs in storvsc. In the face of errors or malicious
behavior in Hyper-V, storvsc should not expose or trust the transaction
IDs returned by Hyper-V to be valid guest memory addresses. Instead,
use small integers generated by vmbus_requestor as requests
(transaction) IDs.

Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
---
 drivers/scsi/storvsc_drv.c | 82 +++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 624467e2590a..38e90675f9c9 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -399,6 +399,7 @@ static int storvsc_timeout = 180;
 static struct scsi_transport_template *fc_transport_template;
 #endif
 
+static struct scsi_host_template scsi_driver;
 static void storvsc_on_channel_callback(void *context);
 
 #define STORVSC_MAX_LUNS_PER_TARGET			255
@@ -698,6 +699,12 @@ static void handle_sc_creation(struct vmbus_channel *new_sc)
 
 	memset(&props, 0, sizeof(struct vmstorage_channel_properties));
 
+	/*
+	 * The size of vmbus_requestor is an upper bound on the number of requests
+	 * that can be in-progress at any one time across all channels.
+	 */
+	new_sc->rqstor_size = scsi_driver.can_queue;
+
 	ret = vmbus_open(new_sc,
 			 storvsc_ringbuffer_size,
 			 storvsc_ringbuffer_size,
@@ -726,6 +733,7 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
 	struct storvsc_cmd_request *request;
 	struct vstor_packet *vstor_packet;
 	int ret, t;
+	u64 rqst_id;
 
 	/*
 	 * If the number of CPUs is artificially restricted, such as
@@ -760,14 +768,22 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
 	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
 	vstor_packet->sub_channel_count = num_sc;
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		dev_err(dev, "No request id available\n");
+		return;
+	}
+
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
 			       vmscsi_size_delta),
-			       (unsigned long)request,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 
 	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		dev_err(dev, "Failed to create sub-channel: err=%d\n", ret);
 		return;
 	}
@@ -818,20 +834,30 @@ static int storvsc_execute_vstor_op(struct hv_device *device,
 {
 	struct vstor_packet *vstor_packet;
 	int ret, t;
+	u64 rqst_id;
 
 	vstor_packet = &request->vstor_packet;
 
 	init_completion(&request->wait_event);
 	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		dev_err(&device->device, "No request id available\n");
+		return -EAGAIN;
+	}
+
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
 			       vmscsi_size_delta),
-			       (unsigned long)request,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-	if (ret != 0)
+	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		return ret;
+	}
 
 	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
 	if (t == 0)
@@ -1233,9 +1259,17 @@ static void storvsc_on_channel_callback(void *context)
 	foreach_vmbus_pkt(desc, channel) {
 		void *packet = hv_pkt_data(desc);
 		struct storvsc_cmd_request *request;
+		u64 cmd_rqst;
 
-		request = (struct storvsc_cmd_request *)
-			((unsigned long)desc->trans_id);
+		cmd_rqst = vmbus_request_addr(&channel->requestor,
+					      desc->trans_id);
+		if (cmd_rqst == VMBUS_RQST_ERROR) {
+			dev_err(&device->device,
+				"Incorrect transaction id\n");
+			continue;
+		}
+
+		request = (struct storvsc_cmd_request *)cmd_rqst;
 
 		if (request == &stor_device->init_request ||
 		    request == &stor_device->reset_request) {
@@ -1256,6 +1290,12 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
 
 	memset(&props, 0, sizeof(struct vmstorage_channel_properties));
 
+	/*
+	 * The size of vmbus_requestor is an upper bound on the number of requests
+	 * that can be in-progress at any one time across all channels.
+	 */
+	device->channel->rqstor_size = scsi_driver.can_queue;
+
 	ret = vmbus_open(device->channel,
 			 ring_size,
 			 ring_size,
@@ -1369,6 +1409,7 @@ static int storvsc_do_io(struct hv_device *device,
 	int ret = 0;
 	const struct cpumask *node_mask;
 	int tgt_cpu;
+	u64 rqst_id;
 
 	vstor_packet = &request->vstor_packet;
 	stor_device = get_out_stor_device(device);
@@ -1463,6 +1504,12 @@ static int storvsc_do_io(struct hv_device *device,
 
 	vstor_packet->operation = VSTOR_OPERATION_EXECUTE_SRB;
 
+	rqst_id = vmbus_next_request_id(&outgoing_channel->requestor, (u64)request);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		dev_err(&device->device, "No request id available\n");
+		return -EAGAIN;
+	}
+
 	if (request->payload->range.len) {
 
 		ret = vmbus_sendpacket_mpb_desc(outgoing_channel,
@@ -1470,18 +1517,21 @@ static int storvsc_do_io(struct hv_device *device,
 				vstor_packet,
 				(sizeof(struct vstor_packet) -
 				vmscsi_size_delta),
-				(unsigned long)request);
+				rqst_id);
 	} else {
 		ret = vmbus_sendpacket(outgoing_channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
 				vmscsi_size_delta),
-			       (unsigned long)request,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	}
 
-	if (ret != 0)
+	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&outgoing_channel->requestor, rqst_id);
 		return ret;
+	}
 
 	atomic_inc(&stor_device->num_outstanding_req);
 
@@ -1562,7 +1612,7 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
 	struct storvsc_cmd_request *request;
 	struct vstor_packet *vstor_packet;
 	int ret, t;
-
+	u64 rqst_id;
 
 	stor_device = get_out_stor_device(device);
 	if (!stor_device)
@@ -1577,14 +1627,24 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
 	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
 	vstor_packet->vm_srb.path_id = stor_device->path_id;
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor,
+					(u64)&stor_device->reset_request);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		dev_err(&device->device, "No request id available\n");
+		return FAILED;
+	}
+
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
 				vmscsi_size_delta),
-			       (unsigned long)&stor_device->reset_request,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-	if (ret != 0)
+	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		return FAILED;
+	}
 
 	t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
 	if (t == 0)
-- 
2.25.1

[PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[Andres Beltran]

Currently, pointers to guest memory are passed to Hyper-V as
transaction IDs in netvsc. In the face of errors or malicious
behavior in Hyper-V, netvsc should not expose or trust the transaction
IDs returned by Hyper-V to be valid guest memory addresses. Instead,
use small integers generated by vmbus_requestor as requests
(transaction) IDs.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
---
 drivers/net/hyperv/hyperv_net.h   | 10 +++++
 drivers/net/hyperv/netvsc.c       | 75 +++++++++++++++++++++++++------
 drivers/net/hyperv/rndis_filter.c |  1 +
 include/linux/hyperv.h            |  1 +
 4 files changed, 73 insertions(+), 14 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index abda736e7c7d..14735c98e798 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -847,6 +847,16 @@ struct nvsp_message {
 
 #define NETVSC_XDP_HDRM 256
 
+#define NETVSC_MIN_OUT_MSG_SIZE (sizeof(struct vmpacket_descriptor) + \
+				 sizeof(struct nvsp_message))
+#define NETVSC_MIN_IN_MSG_SIZE sizeof(struct vmpacket_descriptor)
+
+/* Estimated requestor size:
+ * out_ring_size/min_out_msg_size + in_ring_size/min_in_msg_size
+ */
+#define NETVSC_RQSTOR_SIZE (netvsc_ring_bytes / NETVSC_MIN_OUT_MSG_SIZE + \
+			    netvsc_ring_bytes / NETVSC_MIN_IN_MSG_SIZE)
+
 struct multi_send_data {
 	struct sk_buff *skb; /* skb containing the pkt */
 	struct hv_netvsc_packet *pkt; /* netvsc pkt pending */
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 41f5cf0bb997..c73d5aef4436 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -50,7 +50,7 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
 
 	vmbus_sendpacket(dev->channel, init_pkt,
 			       sizeof(struct nvsp_message),
-			       (unsigned long)init_pkt,
+			       VMBUS_RQST_ID_NO_RESPONSE,
 			       VM_PKT_DATA_INBAND, 0);
 }
 
@@ -163,7 +163,7 @@ static void netvsc_revoke_recv_buf(struct hv_device *device,
 		ret = vmbus_sendpacket(device->channel,
 				       revoke_packet,
 				       sizeof(struct nvsp_message),
-				       (unsigned long)revoke_packet,
+				       VMBUS_RQST_ID_NO_RESPONSE,
 				       VM_PKT_DATA_INBAND, 0);
 		/* If the failure is because the channel is rescinded;
 		 * ignore the failure since we cannot send on a rescinded
@@ -213,7 +213,7 @@ static void netvsc_revoke_send_buf(struct hv_device *device,
 		ret = vmbus_sendpacket(device->channel,
 				       revoke_packet,
 				       sizeof(struct nvsp_message),
-				       (unsigned long)revoke_packet,
+				       VMBUS_RQST_ID_NO_RESPONSE,
 				       VM_PKT_DATA_INBAND, 0);
 
 		/* If the failure is because the channel is rescinded;
@@ -304,6 +304,7 @@ static int netvsc_init_buf(struct hv_device *device,
 	unsigned int buf_size;
 	size_t map_words;
 	int ret = 0;
+	u64 rqst_id;
 
 	/* Get receive buffer area. */
 	buf_size = device_info->recv_sections * device_info->recv_section_size;
@@ -350,13 +351,21 @@ static int netvsc_init_buf(struct hv_device *device,
 
 	trace_nvsp_send(ndev, init_packet);
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		netdev_err(ndev, "No request id available\n");
+		goto cleanup;
+	}
+
 	/* Send the gpadl notification request */
 	ret = vmbus_sendpacket(device->channel, init_packet,
 			       sizeof(struct nvsp_message),
-			       (unsigned long)init_packet,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		netdev_err(ndev,
 			"unable to send receive buffer's gpadl to netvsp\n");
 		goto cleanup;
@@ -432,13 +441,21 @@ static int netvsc_init_buf(struct hv_device *device,
 
 	trace_nvsp_send(ndev, init_packet);
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		netdev_err(ndev, "No request id available\n");
+		goto cleanup;
+	}
+
 	/* Send the gpadl notification request */
 	ret = vmbus_sendpacket(device->channel, init_packet,
 			       sizeof(struct nvsp_message),
-			       (unsigned long)init_packet,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		netdev_err(ndev,
 			   "unable to send send buffer's gpadl to netvsp\n");
 		goto cleanup;
@@ -496,6 +513,7 @@ static int negotiate_nvsp_ver(struct hv_device *device,
 {
 	struct net_device *ndev = hv_get_drvdata(device);
 	int ret;
+	u64 rqst_id;
 
 	memset(init_packet, 0, sizeof(struct nvsp_message));
 	init_packet->hdr.msg_type = NVSP_MSG_TYPE_INIT;
@@ -503,15 +521,24 @@ static int negotiate_nvsp_ver(struct hv_device *device,
 	init_packet->msg.init_msg.init.max_protocol_ver = nvsp_ver;
 	trace_nvsp_send(ndev, init_packet);
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		netdev_err(ndev, "No request id available\n");
+		return -EAGAIN;
+	}
+
 	/* Send the init request */
 	ret = vmbus_sendpacket(device->channel, init_packet,
 			       sizeof(struct nvsp_message),
-			       (unsigned long)init_packet,
+			       rqst_id,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 
-	if (ret != 0)
+	if (ret != 0) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&device->channel->requestor, rqst_id);
 		return ret;
+	}
 
 	wait_for_completion(&net_device->channel_init_wait);
 
@@ -542,7 +569,7 @@ static int negotiate_nvsp_ver(struct hv_device *device,
 
 	ret = vmbus_sendpacket(device->channel, init_packet,
 				sizeof(struct nvsp_message),
-				(unsigned long)init_packet,
+				VMBUS_RQST_ID_NO_RESPONSE,
 				VM_PKT_DATA_INBAND, 0);
 
 	return ret;
@@ -599,7 +626,7 @@ static int netvsc_connect_vsp(struct hv_device *device,
 	/* Send the init request */
 	ret = vmbus_sendpacket(device->channel, init_packet,
 				sizeof(struct nvsp_message),
-				(unsigned long)init_packet,
+				VMBUS_RQST_ID_NO_RESPONSE,
 				VM_PKT_DATA_INBAND, 0);
 	if (ret != 0)
 		goto cleanup;
@@ -680,10 +707,19 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
 				    const struct vmpacket_descriptor *desc,
 				    int budget)
 {
-	struct sk_buff *skb = (struct sk_buff *)(unsigned long)desc->trans_id;
+	struct sk_buff *skb;
 	struct net_device_context *ndev_ctx = netdev_priv(ndev);
 	u16 q_idx = 0;
 	int queue_sends;
+	u64 cmd_rqst;
+
+	cmd_rqst = vmbus_request_addr(&channel->requestor, (u64)desc->trans_id);
+	if (cmd_rqst == VMBUS_RQST_ERROR) {
+		netdev_err(ndev, "Incorrect transaction id\n");
+		return;
+	}
+
+	skb = (struct sk_buff *)cmd_rqst;
 
 	/* Notify the layer above us */
 	if (likely(skb)) {
@@ -822,7 +858,7 @@ static inline int netvsc_send_pkt(
 	struct net_device *ndev = hv_get_drvdata(device);
 	struct net_device_context *ndev_ctx = netdev_priv(ndev);
 	struct netdev_queue *txq = netdev_get_tx_queue(ndev, packet->q_idx);
-	u64 req_id;
+	u64 rqst_id;
 	int ret;
 	u32 ring_avail = hv_get_avail_to_write_percent(&out_channel->outbound);
 
@@ -838,13 +874,18 @@ static inline int netvsc_send_pkt(
 	else
 		rpkt->send_buf_section_size = packet->total_data_buflen;
 
-	req_id = (ulong)skb;
 
 	if (out_channel->rescind)
 		return -ENODEV;
 
 	trace_nvsp_send_pkt(ndev, out_channel, rpkt);
 
+	rqst_id = vmbus_next_request_id(&out_channel->requestor, (u64)skb);
+	if (rqst_id == VMBUS_RQST_ERROR) {
+		ret = -EAGAIN;
+		goto ret_check;
+	}
+
 	if (packet->page_buf_cnt) {
 		if (packet->cp_partial)
 			pb += packet->rmsg_pgcnt;
@@ -852,14 +893,15 @@ static inline int netvsc_send_pkt(
 		ret = vmbus_sendpacket_pagebuffer(out_channel,
 						  pb, packet->page_buf_cnt,
 						  &nvmsg, sizeof(nvmsg),
-						  req_id);
+						  rqst_id);
 	} else {
 		ret = vmbus_sendpacket(out_channel,
 				       &nvmsg, sizeof(nvmsg),
-				       req_id, VM_PKT_DATA_INBAND,
+				       rqst_id, VM_PKT_DATA_INBAND,
 				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
 	}
 
+ret_check:
 	if (ret == 0) {
 		atomic_inc_return(&nvchan->queue_sends);
 
@@ -868,9 +910,13 @@ static inline int netvsc_send_pkt(
 			ndev_ctx->eth_stats.stop_queue++;
 		}
 	} else if (ret == -EAGAIN) {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&out_channel->requestor, rqst_id);
 		netif_tx_stop_queue(txq);
 		ndev_ctx->eth_stats.stop_queue++;
 	} else {
+		/* Reclaim request ID to avoid leak of IDs */
+		vmbus_request_addr(&out_channel->requestor, rqst_id);
 		netdev_err(ndev,
 			   "Unable to send packet pages %u len %u, ret %d\n",
 			   packet->page_buf_cnt, packet->total_data_buflen,
@@ -1422,6 +1468,7 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
 		       netvsc_poll, NAPI_POLL_WEIGHT);
 
 	/* Open the channel */
+	device->channel->rqstor_size = NETVSC_RQSTOR_SIZE;
 	ret = vmbus_open(device->channel, netvsc_ring_bytes,
 			 netvsc_ring_bytes,  NULL, 0,
 			 netvsc_channel_cb, net_device->chan_table);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index b81ceba38218..4d96e8e5ea24 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1114,6 +1114,7 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
 	/* Set the channel before opening.*/
 	nvchan->channel = new_sc;
 
+	new_sc->rqstor_size = NETVSC_RQSTOR_SIZE;
 	ret = vmbus_open(new_sc, netvsc_ring_bytes,
 			 netvsc_ring_bytes, NULL, 0,
 			 netvsc_channel_cb, nvchan);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index c509d20ab7db..d8194924983d 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -730,6 +730,7 @@ struct vmbus_requestor {
 };
 
 #define VMBUS_RQST_ERROR U64_MAX
+#define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 1)
 
 struct vmbus_device {
 	u16  dev_type;
-- 
2.25.1

Re: [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[Andrea Parri]

> Andres Beltran (3):
>   Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
>     hardening
>   scsi: storvsc: Use vmbus_requestor to generate transaction ids for
>     VMBus hardening
>   hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
>     hardening

For the series,

Tested-by: Andrea Parri <parri.andrea@gmail.com>

Thanks,
  Andrea

RE: [PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[Haiyang Zhang]

> -----Original Message-----
> From: Andres Beltran <lkmlabelt@gmail.com>
> Sent: Thursday, June 25, 2020 11:37 AM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> wei.liu@kernel.org
> Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Michael
> Kelley <mikelley@microsoft.com>; parri.andrea@gmail.com; Andres Beltran
> <lkmlabelt@gmail.com>; David S. Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org
> Subject: [PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction
> IDs for VMBus hardening
> 
> Currently, pointers to guest memory are passed to Hyper-V as
> transaction IDs in netvsc. In the face of errors or malicious
> behavior in Hyper-V, netvsc should not expose or trust the transaction
> IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> use small integers generated by vmbus_requestor as requests
> (transaction) IDs.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> ---
>  drivers/net/hyperv/hyperv_net.h   | 10 +++++
>  drivers/net/hyperv/netvsc.c       | 75 +++++++++++++++++++++++++------
>  drivers/net/hyperv/rndis_filter.c |  1 +
>  include/linux/hyperv.h            |  1 +
>  4 files changed, 73 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index abda736e7c7d..14735c98e798 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -847,6 +847,16 @@ struct nvsp_message {
> 
>  #define NETVSC_XDP_HDRM 256
> 
> +#define NETVSC_MIN_OUT_MSG_SIZE (sizeof(struct vmpacket_descriptor) + \
> +				 sizeof(struct nvsp_message))
> +#define NETVSC_MIN_IN_MSG_SIZE sizeof(struct vmpacket_descriptor)
> +
> +/* Estimated requestor size:
> + * out_ring_size/min_out_msg_size + in_ring_size/min_in_msg_size
> + */
> +#define NETVSC_RQSTOR_SIZE (netvsc_ring_bytes /
> NETVSC_MIN_OUT_MSG_SIZE + \
> +			    netvsc_ring_bytes / NETVSC_MIN_IN_MSG_SIZE)

Please make the variable as the macro parameter for readability:
#define NETVSC_RQSTOR_SIZE(ringbytes) (ringbytes / NETVSC_MIN_OUT_MSG_SIZE ...

Then put the actual variable name when you use the macro.

Thanks,
- Haiyang

Re: [PATCH 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5878 bytes --]

Hi Andres,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200625]
[also build test WARNING on v5.8-rc2]
[cannot apply to mkp-scsi/for-next scsi/for-next linux/master linus/master v5.8-rc2 v5.8-rc1 v5.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andres-Beltran/Drivers-hv-vmbus-vmbus_requestor-data-structure/20200625-234113
base:    3f9437c6234d95d96967f1b438a4fb71b6be254d
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/scsi/storvsc_drv.c: In function 'handle_multichannel_storage':
>> drivers/scsi/storvsc_drv.c:771:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     771 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
         |                                                               ^
   drivers/scsi/storvsc_drv.c: In function 'storvsc_execute_vstor_op':
   drivers/scsi/storvsc_drv.c:844:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     844 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
         |                                                               ^
   drivers/scsi/storvsc_drv.c: In function 'storvsc_on_channel_callback':
>> drivers/scsi/storvsc_drv.c:1272:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
    1272 |   request = (struct storvsc_cmd_request *)cmd_rqst;
         |             ^
   drivers/scsi/storvsc_drv.c: In function 'storvsc_do_io':
   drivers/scsi/storvsc_drv.c:1507:64: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1507 |  rqst_id = vmbus_next_request_id(&outgoing_channel->requestor, (u64)request);
         |                                                                ^
   drivers/scsi/storvsc_drv.c: In function 'storvsc_host_reset_handler':
   drivers/scsi/storvsc_drv.c:1631:6: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
    1631 |      (u64)&stor_device->reset_request);
         |      ^

vim +771 drivers/scsi/storvsc_drv.c

   727	
   728	static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
   729	{
   730		struct device *dev = &device->device;
   731		struct storvsc_device *stor_device;
   732		int num_sc;
   733		struct storvsc_cmd_request *request;
   734		struct vstor_packet *vstor_packet;
   735		int ret, t;
   736		u64 rqst_id;
   737	
   738		/*
   739		 * If the number of CPUs is artificially restricted, such as
   740		 * with maxcpus=1 on the kernel boot line, Hyper-V could offer
   741		 * sub-channels >= the number of CPUs. These sub-channels
   742		 * should not be created. The primary channel is already created
   743		 * and assigned to one CPU, so check against # CPUs - 1.
   744		 */
   745		num_sc = min((int)(num_online_cpus() - 1), max_chns);
   746		if (!num_sc)
   747			return;
   748	
   749		stor_device = get_out_stor_device(device);
   750		if (!stor_device)
   751			return;
   752	
   753		stor_device->num_sc = num_sc;
   754		request = &stor_device->init_request;
   755		vstor_packet = &request->vstor_packet;
   756	
   757		/*
   758		 * Establish a handler for dealing with subchannels.
   759		 */
   760		vmbus_set_sc_create_callback(device->channel, handle_sc_creation);
   761	
   762		/*
   763		 * Request the host to create sub-channels.
   764		 */
   765		memset(request, 0, sizeof(struct storvsc_cmd_request));
   766		init_completion(&request->wait_event);
   767		vstor_packet->operation = VSTOR_OPERATION_CREATE_SUB_CHANNELS;
   768		vstor_packet->flags = REQUEST_COMPLETION_FLAG;
   769		vstor_packet->sub_channel_count = num_sc;
   770	
 > 771		rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
   772		if (rqst_id == VMBUS_RQST_ERROR) {
   773			dev_err(dev, "No request id available\n");
   774			return;
   775		}
   776	
   777		ret = vmbus_sendpacket(device->channel, vstor_packet,
   778				       (sizeof(struct vstor_packet) -
   779				       vmscsi_size_delta),
   780				       rqst_id,
   781				       VM_PKT_DATA_INBAND,
   782				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
   783	
   784		if (ret != 0) {
   785			/* Reclaim request ID to avoid leak of IDs */
   786			vmbus_request_addr(&device->channel->requestor, rqst_id);
   787			dev_err(dev, "Failed to create sub-channel: err=%d\n", ret);
   788			return;
   789		}
   790	
   791		t = wait_for_completion_timeout(&request->wait_event, 10*HZ);
   792		if (t == 0) {
   793			dev_err(dev, "Failed to create sub-channel: timed out\n");
   794			return;
   795		}
   796	
   797		if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
   798		    vstor_packet->status != 0) {
   799			dev_err(dev, "Failed to create sub-channel: op=%d, sts=%d\n",
   800				vstor_packet->operation, vstor_packet->status);
   801			return;
   802		}
   803	
   804		/*
   805		 * We need to do nothing here, because vmbus_process_offer()
   806		 * invokes channel->sc_creation_callback, which will open and use
   807		 * the sub-channel(s).
   808		 */
   809	}
   810	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 74226 bytes --]

Re: [PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10860 bytes --]

Hi Andres,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200625]
[also build test WARNING on v5.8-rc2]
[cannot apply to mkp-scsi/for-next scsi/for-next linux/master linus/master v5.8-rc2 v5.8-rc1 v5.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andres-Beltran/Drivers-hv-vmbus-vmbus_requestor-data-structure/20200625-234113
base:    3f9437c6234d95d96967f1b438a4fb71b6be254d
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/hyperv/netvsc.c: In function 'netvsc_init_buf':
>> drivers/net/hyperv/netvsc.c:354:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     354 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c:444:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     444 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c: In function 'negotiate_nvsp_ver':
   drivers/net/hyperv/netvsc.c:524:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     524 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c: In function 'netvsc_send_tx_complete':
>> drivers/net/hyperv/netvsc.c:722:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     722 |  skb = (struct sk_buff *)cmd_rqst;
         |        ^
   drivers/net/hyperv/netvsc.c: In function 'netvsc_send_pkt':
   drivers/net/hyperv/netvsc.c:883:59: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     883 |  rqst_id = vmbus_next_request_id(&out_channel->requestor, (u64)skb);
         |                                                           ^

vim +354 drivers/net/hyperv/netvsc.c

   296	
   297	static int netvsc_init_buf(struct hv_device *device,
   298				   struct netvsc_device *net_device,
   299				   const struct netvsc_device_info *device_info)
   300	{
   301		struct nvsp_1_message_send_receive_buffer_complete *resp;
   302		struct net_device *ndev = hv_get_drvdata(device);
   303		struct nvsp_message *init_packet;
   304		unsigned int buf_size;
   305		size_t map_words;
   306		int ret = 0;
   307		u64 rqst_id;
   308	
   309		/* Get receive buffer area. */
   310		buf_size = device_info->recv_sections * device_info->recv_section_size;
   311		buf_size = roundup(buf_size, PAGE_SIZE);
   312	
   313		/* Legacy hosts only allow smaller receive buffer */
   314		if (net_device->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
   315			buf_size = min_t(unsigned int, buf_size,
   316					 NETVSC_RECEIVE_BUFFER_SIZE_LEGACY);
   317	
   318		net_device->recv_buf = vzalloc(buf_size);
   319		if (!net_device->recv_buf) {
   320			netdev_err(ndev,
   321				   "unable to allocate receive buffer of size %u\n",
   322				   buf_size);
   323			ret = -ENOMEM;
   324			goto cleanup;
   325		}
   326	
   327		net_device->recv_buf_size = buf_size;
   328	
   329		/*
   330		 * Establish the gpadl handle for this buffer on this
   331		 * channel.  Note: This call uses the vmbus connection rather
   332		 * than the channel to establish the gpadl handle.
   333		 */
   334		ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
   335					    buf_size,
   336					    &net_device->recv_buf_gpadl_handle);
   337		if (ret != 0) {
   338			netdev_err(ndev,
   339				"unable to establish receive buffer's gpadl\n");
   340			goto cleanup;
   341		}
   342	
   343		/* Notify the NetVsp of the gpadl handle */
   344		init_packet = &net_device->channel_init_pkt;
   345		memset(init_packet, 0, sizeof(struct nvsp_message));
   346		init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_RECV_BUF;
   347		init_packet->msg.v1_msg.send_recv_buf.
   348			gpadl_handle = net_device->recv_buf_gpadl_handle;
   349		init_packet->msg.v1_msg.
   350			send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
   351	
   352		trace_nvsp_send(ndev, init_packet);
   353	
 > 354		rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
   355		if (rqst_id == VMBUS_RQST_ERROR) {
   356			netdev_err(ndev, "No request id available\n");
   357			goto cleanup;
   358		}
   359	
   360		/* Send the gpadl notification request */
   361		ret = vmbus_sendpacket(device->channel, init_packet,
   362				       sizeof(struct nvsp_message),
   363				       rqst_id,
   364				       VM_PKT_DATA_INBAND,
   365				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
   366		if (ret != 0) {
   367			/* Reclaim request ID to avoid leak of IDs */
   368			vmbus_request_addr(&device->channel->requestor, rqst_id);
   369			netdev_err(ndev,
   370				"unable to send receive buffer's gpadl to netvsp\n");
   371			goto cleanup;
   372		}
   373	
   374		wait_for_completion(&net_device->channel_init_wait);
   375	
   376		/* Check the response */
   377		resp = &init_packet->msg.v1_msg.send_recv_buf_complete;
   378		if (resp->status != NVSP_STAT_SUCCESS) {
   379			netdev_err(ndev,
   380				   "Unable to complete receive buffer initialization with NetVsp - status %d\n",
   381				   resp->status);
   382			ret = -EINVAL;
   383			goto cleanup;
   384		}
   385	
   386		/* Parse the response */
   387		netdev_dbg(ndev, "Receive sections: %u sub_allocs: size %u count: %u\n",
   388			   resp->num_sections, resp->sections[0].sub_alloc_size,
   389			   resp->sections[0].num_sub_allocs);
   390	
   391		/* There should only be one section for the entire receive buffer */
   392		if (resp->num_sections != 1 || resp->sections[0].offset != 0) {
   393			ret = -EINVAL;
   394			goto cleanup;
   395		}
   396	
   397		net_device->recv_section_size = resp->sections[0].sub_alloc_size;
   398		net_device->recv_section_cnt = resp->sections[0].num_sub_allocs;
   399	
   400		/* Setup receive completion ring.
   401		 * Add 1 to the recv_section_cnt because at least one entry in a
   402		 * ring buffer has to be empty.
   403		 */
   404		net_device->recv_completion_cnt = net_device->recv_section_cnt + 1;
   405		ret = netvsc_alloc_recv_comp_ring(net_device, 0);
   406		if (ret)
   407			goto cleanup;
   408	
   409		/* Now setup the send buffer. */
   410		buf_size = device_info->send_sections * device_info->send_section_size;
   411		buf_size = round_up(buf_size, PAGE_SIZE);
   412	
   413		net_device->send_buf = vzalloc(buf_size);
   414		if (!net_device->send_buf) {
   415			netdev_err(ndev, "unable to allocate send buffer of size %u\n",
   416				   buf_size);
   417			ret = -ENOMEM;
   418			goto cleanup;
   419		}
   420	
   421		/* Establish the gpadl handle for this buffer on this
   422		 * channel.  Note: This call uses the vmbus connection rather
   423		 * than the channel to establish the gpadl handle.
   424		 */
   425		ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
   426					    buf_size,
   427					    &net_device->send_buf_gpadl_handle);
   428		if (ret != 0) {
   429			netdev_err(ndev,
   430				   "unable to establish send buffer's gpadl\n");
   431			goto cleanup;
   432		}
   433	
   434		/* Notify the NetVsp of the gpadl handle */
   435		init_packet = &net_device->channel_init_pkt;
   436		memset(init_packet, 0, sizeof(struct nvsp_message));
   437		init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
   438		init_packet->msg.v1_msg.send_send_buf.gpadl_handle =
   439			net_device->send_buf_gpadl_handle;
   440		init_packet->msg.v1_msg.send_send_buf.id = NETVSC_SEND_BUFFER_ID;
   441	
   442		trace_nvsp_send(ndev, init_packet);
   443	
   444		rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
   445		if (rqst_id == VMBUS_RQST_ERROR) {
   446			netdev_err(ndev, "No request id available\n");
   447			goto cleanup;
   448		}
   449	
   450		/* Send the gpadl notification request */
   451		ret = vmbus_sendpacket(device->channel, init_packet,
   452				       sizeof(struct nvsp_message),
   453				       rqst_id,
   454				       VM_PKT_DATA_INBAND,
   455				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
   456		if (ret != 0) {
   457			/* Reclaim request ID to avoid leak of IDs */
   458			vmbus_request_addr(&device->channel->requestor, rqst_id);
   459			netdev_err(ndev,
   460				   "unable to send send buffer's gpadl to netvsp\n");
   461			goto cleanup;
   462		}
   463	
   464		wait_for_completion(&net_device->channel_init_wait);
   465	
   466		/* Check the response */
   467		if (init_packet->msg.v1_msg.
   468		    send_send_buf_complete.status != NVSP_STAT_SUCCESS) {
   469			netdev_err(ndev, "Unable to complete send buffer "
   470				   "initialization with NetVsp - status %d\n",
   471				   init_packet->msg.v1_msg.
   472				   send_send_buf_complete.status);
   473			ret = -EINVAL;
   474			goto cleanup;
   475		}
   476	
   477		/* Parse the response */
   478		net_device->send_section_size = init_packet->msg.
   479					v1_msg.send_send_buf_complete.section_size;
   480	
   481		/* Section count is simply the size divided by the section size. */
   482		net_device->send_section_cnt = buf_size / net_device->send_section_size;
   483	
   484		netdev_dbg(ndev, "Send section size: %d, Section count:%d\n",
   485			   net_device->send_section_size, net_device->send_section_cnt);
   486	
   487		/* Setup state for managing the send buffer. */
   488		map_words = DIV_ROUND_UP(net_device->send_section_cnt, BITS_PER_LONG);
   489	
   490		net_device->send_section_map = kcalloc(map_words, sizeof(ulong), GFP_KERNEL);
   491		if (net_device->send_section_map == NULL) {
   492			ret = -ENOMEM;
   493			goto cleanup;
   494		}
   495	
   496		goto exit;
   497	
   498	cleanup:
   499		netvsc_revoke_recv_buf(device, net_device, ndev);
   500		netvsc_revoke_send_buf(device, net_device, ndev);
   501		netvsc_teardown_recv_gpadl(device, net_device, ndev);
   502		netvsc_teardown_send_gpadl(device, net_device, ndev);
   503	
   504	exit:
   505		return ret;
   506	}
   507	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74226 bytes --]

Re: [PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11124 bytes --]

Hi Andres,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200625]
[also build test WARNING on v5.8-rc2]
[cannot apply to mkp-scsi/for-next scsi/for-next linux/master linus/master v5.8-rc2 v5.8-rc1 v5.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andres-Beltran/Drivers-hv-vmbus-vmbus_requestor-data-structure/20200625-234113
base:    3f9437c6234d95d96967f1b438a4fb71b6be254d
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/hyperv/netvsc.c: In function 'netvsc_init_buf':
>> drivers/net/hyperv/netvsc.c:354:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     354 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c:444:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     444 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c: In function 'negotiate_nvsp_ver':
   drivers/net/hyperv/netvsc.c:524:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     524 |  rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
         |                                                               ^
   drivers/net/hyperv/netvsc.c: In function 'netvsc_send_tx_complete':
>> drivers/net/hyperv/netvsc.c:722:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     722 |  skb = (struct sk_buff *)cmd_rqst;
         |        ^
   drivers/net/hyperv/netvsc.c: In function 'netvsc_send_pkt':
   drivers/net/hyperv/netvsc.c:883:59: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     883 |  rqst_id = vmbus_next_request_id(&out_channel->requestor, (u64)skb);
         |                                                           ^

vim +354 drivers/net/hyperv/netvsc.c

   296	
   297	static int netvsc_init_buf(struct hv_device *device,
   298				   struct netvsc_device *net_device,
   299				   const struct netvsc_device_info *device_info)
   300	{
   301		struct nvsp_1_message_send_receive_buffer_complete *resp;
   302		struct net_device *ndev = hv_get_drvdata(device);
   303		struct nvsp_message *init_packet;
   304		unsigned int buf_size;
   305		size_t map_words;
   306		int ret = 0;
   307		u64 rqst_id;
   308	
   309		/* Get receive buffer area. */
   310		buf_size = device_info->recv_sections * device_info->recv_section_size;
   311		buf_size = roundup(buf_size, PAGE_SIZE);
   312	
   313		/* Legacy hosts only allow smaller receive buffer */
   314		if (net_device->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
   315			buf_size = min_t(unsigned int, buf_size,
   316					 NETVSC_RECEIVE_BUFFER_SIZE_LEGACY);
   317	
   318		net_device->recv_buf = vzalloc(buf_size);
   319		if (!net_device->recv_buf) {
   320			netdev_err(ndev,
   321				   "unable to allocate receive buffer of size %u\n",
   322				   buf_size);
   323			ret = -ENOMEM;
   324			goto cleanup;
   325		}
   326	
   327		net_device->recv_buf_size = buf_size;
   328	
   329		/*
   330		 * Establish the gpadl handle for this buffer on this
   331		 * channel.  Note: This call uses the vmbus connection rather
   332		 * than the channel to establish the gpadl handle.
   333		 */
   334		ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
   335					    buf_size,
   336					    &net_device->recv_buf_gpadl_handle);
   337		if (ret != 0) {
   338			netdev_err(ndev,
   339				"unable to establish receive buffer's gpadl\n");
   340			goto cleanup;
   341		}
   342	
   343		/* Notify the NetVsp of the gpadl handle */
   344		init_packet = &net_device->channel_init_pkt;
   345		memset(init_packet, 0, sizeof(struct nvsp_message));
   346		init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_RECV_BUF;
   347		init_packet->msg.v1_msg.send_recv_buf.
   348			gpadl_handle = net_device->recv_buf_gpadl_handle;
   349		init_packet->msg.v1_msg.
   350			send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
   351	
   352		trace_nvsp_send(ndev, init_packet);
   353	
 > 354		rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
   355		if (rqst_id == VMBUS_RQST_ERROR) {
   356			netdev_err(ndev, "No request id available\n");
   357			goto cleanup;
   358		}
   359	
   360		/* Send the gpadl notification request */
   361		ret = vmbus_sendpacket(device->channel, init_packet,
   362				       sizeof(struct nvsp_message),
   363				       rqst_id,
   364				       VM_PKT_DATA_INBAND,
   365				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
   366		if (ret != 0) {
   367			/* Reclaim request ID to avoid leak of IDs */
   368			vmbus_request_addr(&device->channel->requestor, rqst_id);
   369			netdev_err(ndev,
   370				"unable to send receive buffer's gpadl to netvsp\n");
   371			goto cleanup;
   372		}
   373	
   374		wait_for_completion(&net_device->channel_init_wait);
   375	
   376		/* Check the response */
   377		resp = &init_packet->msg.v1_msg.send_recv_buf_complete;
   378		if (resp->status != NVSP_STAT_SUCCESS) {
   379			netdev_err(ndev,
   380				   "Unable to complete receive buffer initialization with NetVsp - status %d\n",
   381				   resp->status);
   382			ret = -EINVAL;
   383			goto cleanup;
   384		}
   385	
   386		/* Parse the response */
   387		netdev_dbg(ndev, "Receive sections: %u sub_allocs: size %u count: %u\n",
   388			   resp->num_sections, resp->sections[0].sub_alloc_size,
   389			   resp->sections[0].num_sub_allocs);
   390	
   391		/* There should only be one section for the entire receive buffer */
   392		if (resp->num_sections != 1 || resp->sections[0].offset != 0) {
   393			ret = -EINVAL;
   394			goto cleanup;
   395		}
   396	
   397		net_device->recv_section_size = resp->sections[0].sub_alloc_size;
   398		net_device->recv_section_cnt = resp->sections[0].num_sub_allocs;
   399	
   400		/* Setup receive completion ring.
   401		 * Add 1 to the recv_section_cnt because at least one entry in a
   402		 * ring buffer has to be empty.
   403		 */
   404		net_device->recv_completion_cnt = net_device->recv_section_cnt + 1;
   405		ret = netvsc_alloc_recv_comp_ring(net_device, 0);
   406		if (ret)
   407			goto cleanup;
   408	
   409		/* Now setup the send buffer. */
   410		buf_size = device_info->send_sections * device_info->send_section_size;
   411		buf_size = round_up(buf_size, PAGE_SIZE);
   412	
   413		net_device->send_buf = vzalloc(buf_size);
   414		if (!net_device->send_buf) {
   415			netdev_err(ndev, "unable to allocate send buffer of size %u\n",
   416				   buf_size);
   417			ret = -ENOMEM;
   418			goto cleanup;
   419		}
   420	
   421		/* Establish the gpadl handle for this buffer on this
   422		 * channel.  Note: This call uses the vmbus connection rather
   423		 * than the channel to establish the gpadl handle.
   424		 */
   425		ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
   426					    buf_size,
   427					    &net_device->send_buf_gpadl_handle);
   428		if (ret != 0) {
   429			netdev_err(ndev,
   430				   "unable to establish send buffer's gpadl\n");
   431			goto cleanup;
   432		}
   433	
   434		/* Notify the NetVsp of the gpadl handle */
   435		init_packet = &net_device->channel_init_pkt;
   436		memset(init_packet, 0, sizeof(struct nvsp_message));
   437		init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
   438		init_packet->msg.v1_msg.send_send_buf.gpadl_handle =
   439			net_device->send_buf_gpadl_handle;
   440		init_packet->msg.v1_msg.send_send_buf.id = NETVSC_SEND_BUFFER_ID;
   441	
   442		trace_nvsp_send(ndev, init_packet);
   443	
   444		rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
   445		if (rqst_id == VMBUS_RQST_ERROR) {
   446			netdev_err(ndev, "No request id available\n");
   447			goto cleanup;
   448		}
   449	
   450		/* Send the gpadl notification request */
   451		ret = vmbus_sendpacket(device->channel, init_packet,
   452				       sizeof(struct nvsp_message),
   453				       rqst_id,
   454				       VM_PKT_DATA_INBAND,
   455				       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
   456		if (ret != 0) {
   457			/* Reclaim request ID to avoid leak of IDs */
   458			vmbus_request_addr(&device->channel->requestor, rqst_id);
   459			netdev_err(ndev,
   460				   "unable to send send buffer's gpadl to netvsp\n");
   461			goto cleanup;
   462		}
   463	
   464		wait_for_completion(&net_device->channel_init_wait);
   465	
   466		/* Check the response */
   467		if (init_packet->msg.v1_msg.
   468		    send_send_buf_complete.status != NVSP_STAT_SUCCESS) {
   469			netdev_err(ndev, "Unable to complete send buffer "
   470				   "initialization with NetVsp - status %d\n",
   471				   init_packet->msg.v1_msg.
   472				   send_send_buf_complete.status);
   473			ret = -EINVAL;
   474			goto cleanup;
   475		}
   476	
   477		/* Parse the response */
   478		net_device->send_section_size = init_packet->msg.
   479					v1_msg.send_send_buf_complete.section_size;
   480	
   481		/* Section count is simply the size divided by the section size. */
   482		net_device->send_section_cnt = buf_size / net_device->send_section_size;
   483	
   484		netdev_dbg(ndev, "Send section size: %d, Section count:%d\n",
   485			   net_device->send_section_size, net_device->send_section_cnt);
   486	
   487		/* Setup state for managing the send buffer. */
   488		map_words = DIV_ROUND_UP(net_device->send_section_cnt, BITS_PER_LONG);
   489	
   490		net_device->send_section_map = kcalloc(map_words, sizeof(ulong), GFP_KERNEL);
   491		if (net_device->send_section_map == NULL) {
   492			ret = -ENOMEM;
   493			goto cleanup;
   494		}
   495	
   496		goto exit;
   497	
   498	cleanup:
   499		netvsc_revoke_recv_buf(device, net_device, ndev);
   500		netvsc_revoke_send_buf(device, net_device, ndev);
   501		netvsc_teardown_recv_gpadl(device, net_device, ndev);
   502		netvsc_teardown_send_gpadl(device, net_device, ndev);
   503	
   504	exit:
   505		return ret;
   506	}
   507	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 74226 bytes --]

Re: [PATCH 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

[Wei Liu]

On Thu, Jun 25, 2020 at 11:37:21AM -0400, Andres Beltran wrote:
> Currently, VMbus drivers use pointers into guest memory as request IDs
> for interactions with Hyper-V. To be more robust in the face of errors
> or malicious behavior from a compromised Hyper-V, avoid exposing
> guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> bad request ID that is then treated as the address of a guest data
> structure with no validation. Instead, encapsulate these memory
> addresses and provide small integers as request IDs.
> 
> Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> ---
>  drivers/hv/channel.c   | 149 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/hyperv.h |  21 ++++++
>  2 files changed, 170 insertions(+)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 3ebda7707e46..2ea1bfecbfda 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -112,6 +112,70 @@ int vmbus_alloc_ring(struct vmbus_channel *newchannel,
>  }
>  EXPORT_SYMBOL_GPL(vmbus_alloc_ring);
>  
> +/**
> + * request_arr_init - Allocates memory for the requestor array. Each slot
> + * keeps track of the next available slot in the array. Initially, each
> + * slot points to the next one (as in a Linked List). The last slot
> + * does not point to anything, so its value is U64_MAX by default.
> + * @size The size of the array
> + */
> +static u64 *request_arr_init(u32 size)
> +{
> +	int i;
> +	u64 *req_arr;
> +
> +	req_arr = kcalloc(size, sizeof(u64), GFP_KERNEL);
> +	if (!req_arr)
> +		return NULL;
> +
> +	for (i = 0; i < size - 1; i++)
> +		req_arr[i] = i + 1;
> +
> +	/* Last slot (no more available slots) */
> +	req_arr[i] = U64_MAX;
> +
> +	return req_arr;
> +}
> +
> +/*
> + * vmbus_alloc_requestor - Initializes @rqstor's fields.
> + * Slot at index 0 is the first free slot.
> + * @size: Size of the requestor array
> + */
> +static int vmbus_alloc_requestor(struct vmbus_requestor *rqstor, u32 size)
> +{
> +	u64 *rqst_arr;
> +	unsigned long *bitmap;
> +
> +	rqst_arr = request_arr_init(size);
> +	if (!rqst_arr)
> +		return -ENOMEM;
> +
> +	bitmap = bitmap_zalloc(size, GFP_KERNEL);
> +	if (!bitmap) {
> +		kfree(rqst_arr);
> +		return -ENOMEM;
> +	}
> +
> +	rqstor->req_arr = rqst_arr;
> +	rqstor->req_bitmap = bitmap;
> +	rqstor->size = size;
> +	rqstor->next_request_id = 0;
> +	spin_lock_init(&rqstor->req_lock);
> +
> +	return 0;
> +}
> +
> +/*
> + * vmbus_free_requestor - Frees memory allocated for @rqstor
> + * @rqstor: Pointer to the requestor struct
> + */
> +static void vmbus_free_requestor(struct vmbus_requestor *rqstor)
> +{
> +	kfree(rqstor->req_arr);
> +	bitmap_free(rqstor->req_bitmap);
> +}
> +
>  static int __vmbus_open(struct vmbus_channel *newchannel,
>  		       void *userdata, u32 userdatalen,
>  		       void (*onchannelcallback)(void *context), void *context)
> @@ -122,6 +186,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	u32 send_pages, recv_pages;
>  	unsigned long flags;
>  	int err;
> +	int rqstor;
>  
>  	if (userdatalen > MAX_USER_DEFINED_BYTES)
>  		return -EINVAL;
> @@ -132,6 +197,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	if (newchannel->state != CHANNEL_OPEN_STATE)
>  		return -EINVAL;
>  
> +	/* Create and init requestor */
> +	if (newchannel->rqstor_size) {
> +		rqstor = vmbus_alloc_requestor(&newchannel->requestor,
> +					       newchannel->rqstor_size);

You can simply use err here to store the return value or even get rid of
rqstor by doing

   if (vmbus_alloc_requestor(...))

> +		if (rqstor)
> +			return -ENOMEM;
> +	}
> +
>  	newchannel->state = CHANNEL_OPENING_STATE;
>  	newchannel->onchannel_callback = onchannelcallback;
>  	newchannel->channel_callback_context = context;
> @@ -228,6 +301,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  error_clean_ring:
>  	hv_ringbuffer_cleanup(&newchannel->outbound);
>  	hv_ringbuffer_cleanup(&newchannel->inbound);
> +	vmbus_free_requestor(&newchannel->requestor);
>  	newchannel->state = CHANNEL_OPEN_STATE;
>  	return err;
>  }
> @@ -703,6 +777,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
>  		channel->ringbuffer_gpadlhandle = 0;
>  	}
>  
> +	if (!ret)
> +		vmbus_free_requestor(&channel->requestor);
> +
>  	return ret;
>  }
>  
> @@ -937,3 +1014,75 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
>  				  buffer_actual_len, requestid, true);
>  }
>  EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> +
> +/*
> + * vmbus_next_request_id - Returns a new request id. It is also
> + * the index at which the guest memory address is stored.
> + * Uses a spin lock to avoid race conditions.
> + * @rqstor: Pointer to the requestor struct
> + * @rqst_add: Guest memory address to be stored in the array
> + */
> +u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> +{
> +	unsigned long flags;
> +	u64 current_id;
> +
> +	spin_lock_irqsave(&rqstor->req_lock, flags);

Do you really need the irqsave variant here? I.e. is there really a
chance this code is reachable from an interrupt handler?

Wei.

Re: [PATCH 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[Wei Liu]

On Thu, Jun 25, 2020 at 11:37:22AM -0400, Andres Beltran wrote:
>  	 * If the number of CPUs is artificially restricted, such as
> @@ -760,14 +768,22 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
>  	vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>  	vstor_packet->sub_channel_count = num_sc;
>  
> +	rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);

You can cast request to unsigned long to fix the warnings on 32bit.

Wei.

Re: [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[Wei Liu]

On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> From: Andres Beltran (Microsoft) <lkmlabelt@gmail.com>
> 
> Currently, VMbus drivers use pointers into guest memory as request IDs
> for interactions with Hyper-V. To be more robust in the face of errors
> or malicious behavior from a compromised Hyper-V, avoid exposing
> guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> bad request ID that is then treated as the address of a guest data
> structure with no validation. Instead, encapsulate these memory
> addresses and provide small integers as request IDs.
> 
> The first patch creates the definitions for the data structure, provides
> helper methods to generate new IDs and retrieve data, and
> allocates/frees the memory needed for vmbus_requestor.
> 
> The second and third patches make use of vmbus_requestor to send request
> IDs to Hyper-V in storvsc and netvsc respectively.
> 

Per my understanding, this new data structure is per-channel, so it
won't introduce contention on the lock in multi-queue scenario. Have you
done any testing to confirm there is no severe performance regression?

Wei.

> Thanks.
> Andres Beltran
> 
> Cc: linux-scsi@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> 
> Andres Beltran (3):
>   Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
>     hardening
>   scsi: storvsc: Use vmbus_requestor to generate transaction ids for
>     VMBus hardening
>   hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
>     hardening
> 
>  drivers/hv/channel.c              | 150 ++++++++++++++++++++++++++++++
>  drivers/net/hyperv/hyperv_net.h   |  10 ++
>  drivers/net/hyperv/netvsc.c       |  56 +++++++++--
>  drivers/net/hyperv/rndis_filter.c |   1 +
>  drivers/scsi/storvsc_drv.c        |  62 ++++++++++--
>  include/linux/hyperv.h            |  22 +++++
>  6 files changed, 283 insertions(+), 18 deletions(-)
> 
> -- 
> 2.25.1
> 

Re: [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[Andrea Parri]

On Fri, Jun 26, 2020 at 01:42:27PM +0000, Wei Liu wrote:
> On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> > From: Andres Beltran (Microsoft) <lkmlabelt@gmail.com>
> > 
> > Currently, VMbus drivers use pointers into guest memory as request IDs
> > for interactions with Hyper-V. To be more robust in the face of errors
> > or malicious behavior from a compromised Hyper-V, avoid exposing
> > guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> > bad request ID that is then treated as the address of a guest data
> > structure with no validation. Instead, encapsulate these memory
> > addresses and provide small integers as request IDs.
> > 
> > The first patch creates the definitions for the data structure, provides
> > helper methods to generate new IDs and retrieve data, and
> > allocates/frees the memory needed for vmbus_requestor.
> > 
> > The second and third patches make use of vmbus_requestor to send request
> > IDs to Hyper-V in storvsc and netvsc respectively.
> > 
> 
> Per my understanding, this new data structure is per-channel, so it
> won't introduce contention on the lock in multi-queue scenario. Have you
> done any testing to confirm there is no severe performance regression?

I did run some performance tests using our dev pipeline (storage and
network workloads).  I did not find regressions w.r.t. baseline.

  Andrea

Re: [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure

[Wei Liu]

On Fri, Jun 26, 2020 at 04:48:17PM +0200, Andrea Parri wrote:
> On Fri, Jun 26, 2020 at 01:42:27PM +0000, Wei Liu wrote:
> > On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> > > From: Andres Beltran (Microsoft) <lkmlabelt@gmail.com>
> > > 
> > > Currently, VMbus drivers use pointers into guest memory as request IDs
> > > for interactions with Hyper-V. To be more robust in the face of errors
> > > or malicious behavior from a compromised Hyper-V, avoid exposing
> > > guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> > > bad request ID that is then treated as the address of a guest data
> > > structure with no validation. Instead, encapsulate these memory
> > > addresses and provide small integers as request IDs.
> > > 
> > > The first patch creates the definitions for the data structure, provides
> > > helper methods to generate new IDs and retrieve data, and
> > > allocates/frees the memory needed for vmbus_requestor.
> > > 
> > > The second and third patches make use of vmbus_requestor to send request
> > > IDs to Hyper-V in storvsc and netvsc respectively.
> > > 
> > 
> > Per my understanding, this new data structure is per-channel, so it
> > won't introduce contention on the lock in multi-queue scenario. Have you
> > done any testing to confirm there is no severe performance regression?
> 
> I did run some performance tests using our dev pipeline (storage and
> network workloads).  I did not find regressions w.r.t. baseline.

Thanks, that's good to hear.

Wei.

> 
>   Andrea