All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andres Beltran <lkmlabelt@gmail.com>
To: kys@microsoft.com, haiyangz@microsoft.com,
	sthemmin@microsoft.com, wei.liu@kernel.org
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	mikelley@microsoft.com, parri.andrea@gmail.com,
	Andres Beltran <lkmlabelt@gmail.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org
Subject: [PATCH 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening
Date: Thu, 25 Jun 2020 11:37:22 -0400	[thread overview]
Message-ID: <20200625153723.8428-3-lkmlabelt@gmail.com> (raw)
In-Reply-To: <20200625153723.8428-1-lkmlabelt@gmail.com>

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


  parent reply	other threads:[~2020-06-25 15:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-25 15:37 [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure Andres Beltran
2020-06-25 15:37 ` [PATCH 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening Andres Beltran
2020-06-26 13:19   ` Wei Liu
2020-06-25 15:37 ` Andres Beltran [this message]
2020-06-25 22:20   ` [PATCH 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs " kernel test robot
2020-06-26 13:35   ` Wei Liu
2020-06-25 15:37 ` [PATCH 3/3] hv_netvsc: " Andres Beltran
2020-06-25 18:57   ` Haiyang Zhang
2020-06-25 23:37   ` kernel test robot
2020-06-25 23:37     ` kernel test robot
2020-06-25 18:22 ` [PATCH 0/3] Drivers: hv: vmbus: vmbus_requestor data structure Andrea Parri
2020-06-26 13:42 ` Wei Liu
2020-06-26 14:48   ` Andrea Parri
2020-06-26 20:57     ` Wei Liu

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=20200625153723.8428-3-lkmlabelt@gmail.com \
    --to=lkmlabelt@gmail.com \
    --cc=haiyangz@microsoft.com \
    --cc=jejb@linux.ibm.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=mikelley@microsoft.com \
    --cc=parri.andrea@gmail.com \
    --cc=sthemmin@microsoft.com \
    --cc=wei.liu@kernel.org \
    /path/to/YOUR_REPLY

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

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