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

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

[Andrea Parri (Microsoft)]

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.

The series is based on 5.10-rc3.  Changelog in the actual patches.

  Andrea

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>
Cc: linux-scsi@vger.kernel.org
Cc: netdev@vger.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              | 174 ++++++++++++++++++++++++++++--
 drivers/hv/hyperv_vmbus.h         |   3 +-
 drivers/hv/ring_buffer.c          |  29 ++++-
 drivers/net/hyperv/hyperv_net.h   |  13 +++
 drivers/net/hyperv/netvsc.c       |  22 ++--
 drivers/net/hyperv/rndis_filter.c |   1 +
 drivers/scsi/storvsc_drv.c        |  26 ++++-
 include/linux/hyperv.h            |  23 ++++
 8 files changed, 273 insertions(+), 18 deletions(-)

-- 
2.25.1

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

[Andrea Parri (Microsoft)]

From: Andres Beltran <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.

Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
Changes in v7:
        - Move the allocation of the request ID after the data has been
          copied into the ring buffer (cf. 1/3)

Changes in v2:
        - Add casts to unsigned long to fix warnings on 32bit

 drivers/scsi/storvsc_drv.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 0c65fbd41035e..369a6c6266729 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,
@@ -1242,9 +1249,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;
+
+		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)desc->trans_id);
+		request = (struct storvsc_cmd_request *)(unsigned long)cmd_rqst;
 
 		if (request == &stor_device->init_request ||
 		    request == &stor_device->reset_request) {
@@ -1265,6 +1280,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,
@@ -1572,7 +1593,6 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
 	struct vstor_packet *vstor_packet;
 	int ret, t;
 
-
 	stor_device = get_out_stor_device(device);
 	if (!stor_device)
 		return FAILED;
-- 
2.25.1

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

[Wei Liu]

On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> From: Andres Beltran <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.
> 
> Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org

Reviewed-by: Wei Liu <wl@xen.org>

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

[Wei Liu]

On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > From: Andres Beltran <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.
> > 
> > Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> > Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> > Cc: linux-scsi@vger.kernel.org
> 
> Reviewed-by: Wei Liu <wl@xen.org>

Martin already gave his ack back in July. I guess nothing substantial
changed so it should have been carried over?

Wei.

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

[Andrea Parri]

On Fri, Nov 13, 2020 at 06:54:24PM +0000, Wei Liu wrote:
> On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> > On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > > From: Andres Beltran <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.
> > > 
> > > Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> > > Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> > > Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> > > Cc: linux-scsi@vger.kernel.org
> > 
> > Reviewed-by: Wei Liu <wl@xen.org>
> 
> Martin already gave his ack back in July. I guess nothing substantial
> changed so it should have been carried over?

The only change here happened in v7 and consisted in moving the
allocation of the request IDs from the VSC code down into the core
vmbus_sendpacket()&co functions.  As mentioned in v7 cover letter,
this change was applied to ensure that the allocation in question
is performed after the packet is copied into the ring buffer.  On
a positive note, this change greatly reduced the diff of this and
the following (NetVSC) patches.

  Andrea

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

[Wei Liu]

On Fri, Nov 13, 2020 at 10:39:33PM +0100, Andrea Parri wrote:
> On Fri, Nov 13, 2020 at 06:54:24PM +0000, Wei Liu wrote:
> > On Fri, Nov 13, 2020 at 11:33:27AM +0000, Wei Liu wrote:
> > > On Mon, Nov 09, 2020 at 11:04:01AM +0100, Andrea Parri (Microsoft) wrote:
> > > > From: Andres Beltran <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.
> > > > 
> > > > Signed-off-by: Andres Beltran <lkmlabelt@gmail.com>
> > > > Co-developed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > > > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > > > Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> > > > Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> > > > Cc: linux-scsi@vger.kernel.org
> > > 
> > > Reviewed-by: Wei Liu <wl@xen.org>
> > 
> > Martin already gave his ack back in July. I guess nothing substantial
> > changed so it should have been carried over?
> 
> The only change here happened in v7 and consisted in moving the
> allocation of the request IDs from the VSC code down into the core
> vmbus_sendpacket()&co functions.  As mentioned in v7 cover letter,
> this change was applied to ensure that the allocation in question
> is performed after the packet is copied into the ring buffer.  On
> a positive note, this change greatly reduced the diff of this and
> the following (NetVSC) patches.

Martin and James, are you happy with this change? I would assume you are
because that means this patch to storvsc is leaner.

Please give an explicit ack if you can. Thanks.

Wei.

> 
>   Andrea

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

[Martin K. Petersen]

Wei,

> Martin and James, are you happy with this change? I would assume you
> are because that means this patch to storvsc is leaner.

Yes, that's fine.

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

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Wei Liu]

On Mon, Nov 09, 2020 at 11:03:59AM +0100, Andrea Parri (Microsoft) 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.
> 
> The series is based on 5.10-rc3.  Changelog in the actual patches.

Applied to hyperv-next. Thanks.

I also corrected the email address in my reviewed-by tags while
committing -- should've use my @kernel.org address, not @xen.org.

Wei.