* [PATCH v3 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs for VMBus hardening
2020-06-30 15:31 [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening Andres Beltran
@ 2020-06-30 15:31 ` Andres Beltran
2020-06-30 18:37 ` Michael Kelley
2020-06-30 17:12 ` [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure " Andrea Parri
2020-06-30 17:16 ` Stephen Hemminger
2 siblings, 1 reply; 6+ messages in thread
From: Andres Beltran @ 2020-06-30 15:31 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu
Cc: linux-hyperv, linux-kernel, mikelley, parri.andrea, skarade,
Andres Beltran, James E.J. Bottomley, Martin K. Petersen,
linux-scsi
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening
2020-06-30 15:31 [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening Andres Beltran
2020-06-30 15:31 ` [PATCH v3 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs " Andres Beltran
@ 2020-06-30 17:12 ` Andrea Parri
2020-06-30 17:16 ` Stephen Hemminger
2 siblings, 0 replies; 6+ messages in thread
From: Andrea Parri @ 2020-06-30 17:12 UTC (permalink / raw)
To: t-mabelt
Cc: kys, haiyangz, sthemmin, wei.liu, linux-hyperv, linux-kernel,
mikelley, skarade, Andres Beltran, linux-scsi, netdev,
James E . J . Bottomley, Martin K . Petersen, David S . Miller
On Tue, Jun 30, 2020 at 11:31:57AM -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
>
> Tested-by: Andrea Parri <parri.andrea@gmail.com>
Em, I don't expect the changes introduced since v1 to have any observable
effects, but I really don't know: I should be able to complete my testing
of this by tomorrow or so; for now, please just ignore this tag.
Thanks,
Andrea
>
> 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>
>
> 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 | 154 ++++++++++++++++++++++++++++++
> 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, 329 insertions(+), 25 deletions(-)
>
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening
2020-06-30 15:31 [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure for VMBus hardening Andres Beltran
2020-06-30 15:31 ` [PATCH v3 2/3] scsi: storvsc: Use vmbus_requestor to generate transaction IDs " Andres Beltran
2020-06-30 17:12 ` [PATCH v3 0/3] Drivers: hv: vmbus: vmbus_requestor data structure " Andrea Parri
@ 2020-06-30 17:16 ` Stephen Hemminger
2020-06-30 18:13 ` Michael Kelley
2 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2020-06-30 17:16 UTC (permalink / raw)
To: Andres Beltran
Cc: t-mabelt, kys, haiyangz, sthemmin, wei.liu, linux-hyperv,
linux-kernel, mikelley, parri.andrea, skarade, linux-scsi, netdev,
James E . J . Bottomley, Martin K . Petersen, David S . Miller
On Tue, 30 Jun 2020 11:31:57 -0400
Andres Beltran <lkmlabelt@gmail.com> 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
>
> Tested-by: Andrea Parri <parri.andrea@gmail.com>
>
> 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>
>
> 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 | 154 ++++++++++++++++++++++++++++++
> 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, 329 insertions(+), 25 deletions(-)
>
How does this interact with use of the vmbus in usermode by DPDK through hv_uio_generic?
Will it still work?
^ permalink raw reply [flat|nested] 6+ messages in thread