[PATCH v4 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening

[PATCH v4 0/3] Drivers: hv: vmbus: 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.

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: James E.J. Bottomley <jejb@linux.ibm.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: David S. Miller <davem@davemloft.net>

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              | 158 ++++++++++++++++++++++++++++++
 drivers/net/hyperv/hyperv_net.h   |  13 +++
 drivers/net/hyperv/netvsc.c       |  79 ++++++++++++---
 drivers/net/hyperv/rndis_filter.c |   1 +
 drivers/scsi/storvsc_drv.c        |  85 +++++++++++++---
 include/linux/hyperv.h            |  22 +++++
 6 files changed, 333 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v4 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>
---
Changes in v4:
	- Use channel->rqstor_size to check if rqstor has been
	  initialized.
Changes in v3:
        - Check that requestor has been initialized in
          vmbus_next_request_id() and vmbus_request_addr().
Changes in v2:
        - Get rid of "rqstor" variable in __vmbus_open().

 drivers/hv/channel.c   | 158 +++++++++++++++++++++++++++++++++++++++++
 include/linux/hyperv.h |  21 ++++++
 2 files changed, 179 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 3ebda7707e46..c16ddd3e5ce1 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)
@@ -132,6 +196,12 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 	if (newchannel->state != CHANNEL_OPEN_STATE)
 		return -EINVAL;
 
+	/* Create and init requestor */
+	if (newchannel->rqstor_size) {
+		if (vmbus_alloc_requestor(&newchannel->requestor, newchannel->rqstor_size))
+			return -ENOMEM;
+	}
+
 	newchannel->state = CHANNEL_OPENING_STATE;
 	newchannel->onchannel_callback = onchannelcallback;
 	newchannel->channel_callback_context = context;
@@ -228,6 +298,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 +774,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 +1011,87 @@ 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;
+	const struct vmbus_channel *channel =
+		container_of(rqstor, const struct vmbus_channel, requestor);
+
+	/* Check rqstor has been initialized */
+	if (!channel->rqstor_size)
+		return VMBUS_RQST_ERROR;
+
+	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;
+	const struct vmbus_channel *channel =
+		container_of(rqstor, const struct vmbus_channel, requestor);
+
+	/* Check rqstor has been initialized */
+	if (!channel->rqstor_size)
+		return VMBUS_RQST_ERROR;
+
+	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 v4 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>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
Changes in v2:
        - Add casts to unsigned long to fix warnings on 32bit.

 drivers/scsi/storvsc_drv.c | 85 +++++++++++++++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 624467e2590a..6d2df1f0fe6d 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,23 @@ 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,
+					(unsigned long)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 +835,31 @@ 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,
+					(unsigned long)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 +1261,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 *)(unsigned long)cmd_rqst;
 
 		if (request == &stor_device->init_request ||
 		    request == &stor_device->reset_request) {
@@ -1256,6 +1292,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 +1411,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 +1506,13 @@ 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,
+					(unsigned long)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 +1520,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 +1615,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 +1630,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,
+					(unsigned long)&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 v4 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>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes in v2:
        - Add casts to unsigned long to fix warnings on 32bit.
        - Use an inline function to get the requestor size.

 drivers/net/hyperv/hyperv_net.h   | 13 +++++
 drivers/net/hyperv/netvsc.c       | 79 +++++++++++++++++++++++++------
 drivers/net/hyperv/rndis_filter.c |  1 +
 include/linux/hyperv.h            |  1 +
 4 files changed, 80 insertions(+), 14 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index abda736e7c7d..f43b614f2345 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -847,6 +847,19 @@ 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
+ */
+static inline u32 netvsc_rqstor_size(unsigned long ringbytes)
+{
+	return ringbytes / NETVSC_MIN_OUT_MSG_SIZE +
+	       ringbytes / 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..79b907a29433 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,22 @@ static int netvsc_init_buf(struct hv_device *device,
 
 	trace_nvsp_send(ndev, init_packet);
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor,
+					(unsigned long)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 +442,22 @@ static int netvsc_init_buf(struct hv_device *device,
 
 	trace_nvsp_send(ndev, init_packet);
 
+	rqst_id = vmbus_next_request_id(&device->channel->requestor,
+					(unsigned long)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 +515,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 +523,25 @@ 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,
+					(unsigned long)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 +572,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 +629,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 +710,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 *)(unsigned long)cmd_rqst;
 
 	/* Notify the layer above us */
 	if (likely(skb)) {
@@ -822,7 +861,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 +877,19 @@ 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,
+					(unsigned long)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 +897,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 +914,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 +1472,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(netvsc_ring_bytes);
 	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..10489ba44a09 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(netvsc_ring_bytes);
 	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 v4 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

[Wei Liu]

On Tue, Jun 30, 2020 at 08:12:20PM -0400, Andres Beltran wrote:
> 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

James and Martin, FYI I'm going to take this patch via hyperv tree
because it depends on the first patch.

Wei.

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

[Martin K. Petersen]

Andres,

> 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.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[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

Confirming previous results,

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

  Andrea

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

[Michael Kelley]

From: Andres Beltran <lkmlabelt@gmail.com> Sent: Tuesday, June 30, 2020 5:12 PM
> 
> 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>
> ---
> Changes in v4:
> 	- Use channel->rqstor_size to check if rqstor has been
> 	  initialized.
> Changes in v3:
>         - Check that requestor has been initialized in
>           vmbus_next_request_id() and vmbus_request_addr().
> Changes in v2:
>         - Get rid of "rqstor" variable in __vmbus_open().
> 
>  drivers/hv/channel.c   | 158 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/hyperv.h |  21 ++++++
>  2 files changed, 179 insertions(+)

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

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

[Wei Liu]

On Tue, Jun 30, 2020 at 08:12:18PM -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.
> 
> 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: James E.J. Bottomley <jejb@linux.ibm.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: David S. Miller <davem@davemloft.net>
> 
> 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

Series applied to hyperv-next. Thanks.

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

[Nathan Chancellor]

Hi Andres,

On Tue, Jun 30, 2020 at 08:12:20PM -0400, Andres Beltran wrote:
> 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>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
> Changes in v2:
>         - Add casts to unsigned long to fix warnings on 32bit.
> 
>  drivers/scsi/storvsc_drv.c | 85 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 624467e2590a..6d2df1f0fe6d 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,23 @@ 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,
> +					(unsigned long)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 +835,31 @@ 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,
> +					(unsigned long)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 +1261,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 *)(unsigned long)cmd_rqst;
>  
>  		if (request == &stor_device->init_request ||
>  		    request == &stor_device->reset_request) {
> @@ -1256,6 +1292,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 +1411,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 +1506,13 @@ 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,
> +					(unsigned long)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 +1520,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 +1615,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 +1630,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,
> +					(unsigned long)&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
> 

This patch has landed in linux-next as of next-20200707 and now I can no
longer boot the WSL2 lightweight VM.

PS C:\Users\natec> wsl -d ubuntu
The virtual machine or container was forcefully exited.

$ git bisect log
# bad: [5b2a702f85b3285fcde0309aadacc13a36c70fc7] Add linux-next specific files for 20200707
# good: [bfe91da29bfad9941d5d703d45e29f0812a20724] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
git bisect start 'origin/master' 'origin/stable'
# good: [885913a4d03f7f5fbd2c75121ea8c42f58185cc5] Merge remote-tracking branch 'crypto/master'
git bisect good 885913a4d03f7f5fbd2c75121ea8c42f58185cc5
# good: [4a902a00a463f60b1630577a32e142800707c576] Merge remote-tracking branch 'regulator/for-next'
git bisect good 4a902a00a463f60b1630577a32e142800707c576
# good: [e48c950eb83e19d532ea49112211b01c6210377a] Merge remote-tracking branch 'thunderbolt/next'
git bisect good e48c950eb83e19d532ea49112211b01c6210377a
# good: [0a299abc3a2127d9711517904a1e5c751985b5a5] Merge remote-tracking branch 'rtc/rtc-next'
git bisect good 0a299abc3a2127d9711517904a1e5c751985b5a5
# good: [6de62f5629875029fbd8d79d7fa9c45e8dbea966] kcov: make some symbols static
git bisect good 6de62f5629875029fbd8d79d7fa9c45e8dbea966
# bad: [9103b615924bf7594a7651a9777e0cf177201dbd] Merge remote-tracking branch 'auxdisplay/auxdisplay'
git bisect bad 9103b615924bf7594a7651a9777e0cf177201dbd
# good: [ed0e825a5c0f00aec12f79e8aef4b37dbb5a94f1] Merge remote-tracking branch 'kspp/for-next/kspp'
git bisect good ed0e825a5c0f00aec12f79e8aef4b37dbb5a94f1
# good: [563bebf9d7625b579a13b79a4981fdd3097d9bce] Merge remote-tracking branch 'nvmem/for-next'
git bisect good 563bebf9d7625b579a13b79a4981fdd3097d9bce
# good: [efd8e353a542e79995681d98a4849eeeb1ce3809] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening
git bisect good efd8e353a542e79995681d98a4849eeeb1ce3809
# good: [27586ca786a729cda6c807621a1494900a56e7bc] XArray: Handle retry entries within xas_find_marked
git bisect good 27586ca786a729cda6c807621a1494900a56e7bc
# bad: [11478f56f20e3be6d11043b501f3090375af4492] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening
git bisect bad 11478f56f20e3be6d11043b501f3090375af4492
# bad: [8e569d774e1e73afabf1fbf40d11fcb8462ddffa] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardeninggit bisect bad 8e569d774e1e73afabf1fbf40d11fcb8462ddffa
# first bad commit: [8e569d774e1e73afabf1fbf40d11fcb8462ddffa] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening

If I revert this commit, everything works fine:

PS C:\Users\natec> wsl --shutdown
PS C:\Users\natec> wsl -d ubuntu -- /bin/bash
nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ cat /proc/version
Linux version 5.8.0-rc4-next-20200707-microsoft-standard+ (nathan@Ryzen-9-3900X) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP Tue Jul 7 16:35:06 MST 2020
nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ git -C ~/src/linux-next lo -2
0ff017dff922 (HEAD -> master) Revert "scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening"
5b2a702f85b3 (tag: next-20200707, origin/master, origin/HEAD) Add linux-next specific files for 20200707
nathan@Ryzen-9-3900X:/mnt/c/Users/natec$

The kernel was built using the following commands:

$ mkdir -p out/x86_64

$ curl -LSso out/x86_64/.config https://github.com/microsoft/WSL2-Linux-Kernel/raw/linux-msft-wsl-4.19.y/Microsoft/config-wsl

$ scripts/config --file out/x86_64/.config -d RAID6_PQ_BENCHMARK -e NET_9P_VIRTIO

$ make -skj"$(nproc)" O=out/x86_64 olddefconfig bzImage

I don't really know how to get more information than this as WSL seems
rather opaque but I am happy to provide any information.

Cheers,
Nathan

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

[Wei Liu]

On Tue, Jul 07, 2020 at 04:47:00PM -0700, Nathan Chancellor wrote:
> Hi Andres,
> 
> On Tue, Jun 30, 2020 at 08:12:20PM -0400, Andres Beltran wrote:
> > 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>
> > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> > Changes in v2:
> >         - Add casts to unsigned long to fix warnings on 32bit.
> > 
> >  drivers/scsi/storvsc_drv.c | 85 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 74 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 624467e2590a..6d2df1f0fe6d 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,23 @@ 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,
> > +					(unsigned long)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 +835,31 @@ 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,
> > +					(unsigned long)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 +1261,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 *)(unsigned long)cmd_rqst;
> >  
> >  		if (request == &stor_device->init_request ||
> >  		    request == &stor_device->reset_request) {
> > @@ -1256,6 +1292,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 +1411,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 +1506,13 @@ 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,
> > +					(unsigned long)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 +1520,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 +1615,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 +1630,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,
> > +					(unsigned long)&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
> > 
> 
> This patch has landed in linux-next as of next-20200707 and now I can no
> longer boot the WSL2 lightweight VM.
> 
> PS C:\Users\natec> wsl -d ubuntu
> The virtual machine or container was forcefully exited.
> 
> $ git bisect log
> # bad: [5b2a702f85b3285fcde0309aadacc13a36c70fc7] Add linux-next specific files for 20200707
> # good: [bfe91da29bfad9941d5d703d45e29f0812a20724] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
> git bisect start 'origin/master' 'origin/stable'
> # good: [885913a4d03f7f5fbd2c75121ea8c42f58185cc5] Merge remote-tracking branch 'crypto/master'
> git bisect good 885913a4d03f7f5fbd2c75121ea8c42f58185cc5
> # good: [4a902a00a463f60b1630577a32e142800707c576] Merge remote-tracking branch 'regulator/for-next'
> git bisect good 4a902a00a463f60b1630577a32e142800707c576
> # good: [e48c950eb83e19d532ea49112211b01c6210377a] Merge remote-tracking branch 'thunderbolt/next'
> git bisect good e48c950eb83e19d532ea49112211b01c6210377a
> # good: [0a299abc3a2127d9711517904a1e5c751985b5a5] Merge remote-tracking branch 'rtc/rtc-next'
> git bisect good 0a299abc3a2127d9711517904a1e5c751985b5a5
> # good: [6de62f5629875029fbd8d79d7fa9c45e8dbea966] kcov: make some symbols static
> git bisect good 6de62f5629875029fbd8d79d7fa9c45e8dbea966
> # bad: [9103b615924bf7594a7651a9777e0cf177201dbd] Merge remote-tracking branch 'auxdisplay/auxdisplay'
> git bisect bad 9103b615924bf7594a7651a9777e0cf177201dbd
> # good: [ed0e825a5c0f00aec12f79e8aef4b37dbb5a94f1] Merge remote-tracking branch 'kspp/for-next/kspp'
> git bisect good ed0e825a5c0f00aec12f79e8aef4b37dbb5a94f1
> # good: [563bebf9d7625b579a13b79a4981fdd3097d9bce] Merge remote-tracking branch 'nvmem/for-next'
> git bisect good 563bebf9d7625b579a13b79a4981fdd3097d9bce
> # good: [efd8e353a542e79995681d98a4849eeeb1ce3809] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening
> git bisect good efd8e353a542e79995681d98a4849eeeb1ce3809
> # good: [27586ca786a729cda6c807621a1494900a56e7bc] XArray: Handle retry entries within xas_find_marked
> git bisect good 27586ca786a729cda6c807621a1494900a56e7bc
> # bad: [11478f56f20e3be6d11043b501f3090375af4492] hv_netvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening
> git bisect bad 11478f56f20e3be6d11043b501f3090375af4492
> # bad: [8e569d774e1e73afabf1fbf40d11fcb8462ddffa] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardeninggit bisect bad 8e569d774e1e73afabf1fbf40d11fcb8462ddffa
> # first bad commit: [8e569d774e1e73afabf1fbf40d11fcb8462ddffa] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening
> 
> If I revert this commit, everything works fine:
> 
> PS C:\Users\natec> wsl --shutdown
> PS C:\Users\natec> wsl -d ubuntu -- /bin/bash
> nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ cat /proc/version
> Linux version 5.8.0-rc4-next-20200707-microsoft-standard+ (nathan@Ryzen-9-3900X) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP Tue Jul 7 16:35:06 MST 2020
> nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ git -C ~/src/linux-next lo -2
> 0ff017dff922 (HEAD -> master) Revert "scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening"
> 5b2a702f85b3 (tag: next-20200707, origin/master, origin/HEAD) Add linux-next specific files for 20200707
> nathan@Ryzen-9-3900X:/mnt/c/Users/natec$
> 
> The kernel was built using the following commands:
> 
> $ mkdir -p out/x86_64
> 
> $ curl -LSso out/x86_64/.config https://github.com/microsoft/WSL2-Linux-Kernel/raw/linux-msft-wsl-4.19.y/Microsoft/config-wsl
> 
> $ scripts/config --file out/x86_64/.config -d RAID6_PQ_BENCHMARK -e NET_9P_VIRTIO
> 
> $ make -skj"$(nproc)" O=out/x86_64 olddefconfig bzImage
> 
> I don't really know how to get more information than this as WSL seems
> rather opaque but I am happy to provide any information.

Linux kernel uses Hyper-V's crash reporting facility to spit out
information when it dies. It is said that you can see that information
in the "Event Viewer" program.

(I've never tried this though -- not using WSL2)

Wei.

> 
> Cheers,
> Nathan

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

[Wei Liu]

On Wed, Jul 08, 2020 at 09:21:05AM +0000, Wei Liu wrote:
[...]
> > If I revert this commit, everything works fine:
> > 
> > PS C:\Users\natec> wsl --shutdown
> > PS C:\Users\natec> wsl -d ubuntu -- /bin/bash
> > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ cat /proc/version
> > Linux version 5.8.0-rc4-next-20200707-microsoft-standard+ (nathan@Ryzen-9-3900X) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP Tue Jul 7 16:35:06 MST 2020
> > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ git -C ~/src/linux-next lo -2
> > 0ff017dff922 (HEAD -> master) Revert "scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening"
> > 5b2a702f85b3 (tag: next-20200707, origin/master, origin/HEAD) Add linux-next specific files for 20200707
> > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$
> > 
> > The kernel was built using the following commands:
> > 
> > $ mkdir -p out/x86_64
> > 
> > $ curl -LSso out/x86_64/.config https://github.com/microsoft/WSL2-Linux-Kernel/raw/linux-msft-wsl-4.19.y/Microsoft/config-wsl
> > 
> > $ scripts/config --file out/x86_64/.config -d RAID6_PQ_BENCHMARK -e NET_9P_VIRTIO
> > 
> > $ make -skj"$(nproc)" O=out/x86_64 olddefconfig bzImage
> > 
> > I don't really know how to get more information than this as WSL seems
> > rather opaque but I am happy to provide any information.
> 
> Linux kernel uses Hyper-V's crash reporting facility to spit out
> information when it dies. It is said that you can see that information
> in the "Event Viewer" program.
> 
> (I've never tried this though -- not using WSL2)
> 

If this doesn't work, another idea is to install a traditional VM on
Hyper-V and replace the kernel with your own.

With such setup, you should be able to add an emulated serial port to
the VM and grab more information.

Wei.

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

[Wei Liu]

On Wed, Jul 08, 2020 at 09:25:12AM +0000, Wei Liu wrote:
> On Wed, Jul 08, 2020 at 09:21:05AM +0000, Wei Liu wrote:
> [...]
> > > If I revert this commit, everything works fine:
> > > 
> > > PS C:\Users\natec> wsl --shutdown
> > > PS C:\Users\natec> wsl -d ubuntu -- /bin/bash
> > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ cat /proc/version
> > > Linux version 5.8.0-rc4-next-20200707-microsoft-standard+ (nathan@Ryzen-9-3900X) (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP Tue Jul 7 16:35:06 MST 2020
> > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ git -C ~/src/linux-next lo -2
> > > 0ff017dff922 (HEAD -> master) Revert "scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening"
> > > 5b2a702f85b3 (tag: next-20200707, origin/master, origin/HEAD) Add linux-next specific files for 20200707
> > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$
> > > 
> > > The kernel was built using the following commands:
> > > 
> > > $ mkdir -p out/x86_64
> > > 
> > > $ curl -LSso out/x86_64/.config https://github.com/microsoft/WSL2-Linux-Kernel/raw/linux-msft-wsl-4.19.y/Microsoft/config-wsl
> > > 
> > > $ scripts/config --file out/x86_64/.config -d RAID6_PQ_BENCHMARK -e NET_9P_VIRTIO
> > > 
> > > $ make -skj"$(nproc)" O=out/x86_64 olddefconfig bzImage
> > > 
> > > I don't really know how to get more information than this as WSL seems
> > > rather opaque but I am happy to provide any information.
> > 
> > Linux kernel uses Hyper-V's crash reporting facility to spit out
> > information when it dies. It is said that you can see that information
> > in the "Event Viewer" program.
> > 
> > (I've never tried this though -- not using WSL2)
> > 
> 
> If this doesn't work, another idea is to install a traditional VM on
> Hyper-V and replace the kernel with your own.
> 
> With such setup, you should be able to add an emulated serial port to
> the VM and grab more information.

Hi Nathan, do you need more help on this?

MSFT is also working on reproducing this internally.

We're ~2 weeks away from the next merge window so it would be good if we
can get to the bottom of this as quickly as possible.

Wei.

> 
> Wei.

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

[Michael Kelley]

From: Wei Liu <wei.liu@kernel.org>  Sent: Friday, July 17, 2020 3:46 AM
> On Wed, Jul 08, 2020 at 09:25:12AM +0000, Wei Liu wrote:
> > On Wed, Jul 08, 2020 at 09:21:05AM +0000, Wei Liu wrote:
> > [...]
> > > > If I revert this commit, everything works fine:
> > > >
> > > > PS C:\Users\natec> wsl --shutdown
> > > > PS C:\Users\natec> wsl -d ubuntu -- /bin/bash
> > > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ cat /proc/version
> > > > Linux version 5.8.0-rc4-next-20200707-microsoft-standard+ (nathan@Ryzen-9-3900X)
> (gcc (Ubuntu 9.3.0-10ubuntu2) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #1 SMP Tue Jul
> 7 16:35:06 MST 2020
> > > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$ git -C ~/src/linux-next lo -2
> > > > 0ff017dff922 (HEAD -> master) Revert "scsi: storvsc: Use vmbus_requestor to
> generate transaction IDs for VMBus hardening"
> > > > 5b2a702f85b3 (tag: next-20200707, origin/master, origin/HEAD) Add linux-next specific
> files for 20200707
> > > > nathan@Ryzen-9-3900X:/mnt/c/Users/natec$
> > > >
> > > > The kernel was built using the following commands:
> > > >
> > > > $ mkdir -p out/x86_64
> > > >
> > > > $ curl -LSso out/x86_64/.config
> https://raw.githubusercontent.com/microsoft/WSL2-Linux-Kernel/linux-msft-wsl-4.19.y/Microsoft/config-wsl
> > > >
> > > > $ scripts/config --file out/x86_64/.config -d RAID6_PQ_BENCHMARK -e
> NET_9P_VIRTIO
> > > >
> > > > $ make -skj"$(nproc)" O=out/x86_64 olddefconfig bzImage
> > > >
> > > > I don't really know how to get more information than this as WSL seems
> > > > rather opaque but I am happy to provide any information.
> > >
> > > Linux kernel uses Hyper-V's crash reporting facility to spit out
> > > information when it dies. It is said that you can see that information
> > > in the "Event Viewer" program.
> > >
> > > (I've never tried this though -- not using WSL2)
> > >
> >
> > If this doesn't work, another idea is to install a traditional VM on
> > Hyper-V and replace the kernel with your own.
> >
> > With such setup, you should be able to add an emulated serial port to
> > the VM and grab more information.
> 
> Hi Nathan, do you need more help on this?
> 
> MSFT is also working on reproducing this internally.
> 
> We're ~2 weeks away from the next merge window so it would be good if we
> can get to the bottom of this as quickly as possible.
> 

On the Microsoft side we now have a repro of the problem when running
in WSLv2.  The symptoms match exactly what Nathan has reported.  We
will debug it from here.  Thanks for reporting this!

Michael