From: Andres Beltran (Microsoft) <[email protected]>
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: [email protected]
Cc: [email protected]
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Andres Beltran (3):
Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
hardening
scsi: storvsc: Use vmbus_requestor to generate transaction ids for
VMBus hardening
hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
hardening
drivers/hv/channel.c | 150 ++++++++++++++++++++++++++++++
drivers/net/hyperv/hyperv_net.h | 10 ++
drivers/net/hyperv/netvsc.c | 56 +++++++++--
drivers/net/hyperv/rndis_filter.c | 1 +
drivers/scsi/storvsc_drv.c | 62 ++++++++++--
include/linux/hyperv.h | 22 +++++
6 files changed, 283 insertions(+), 18 deletions(-)
--
2.25.1
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 <[email protected]>
---
drivers/hv/channel.c | 149 +++++++++++++++++++++++++++++++++++++++++
include/linux/hyperv.h | 21 ++++++
2 files changed, 170 insertions(+)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 3ebda7707e46..2ea1bfecbfda 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -112,6 +112,70 @@ int vmbus_alloc_ring(struct vmbus_channel *newchannel,
}
EXPORT_SYMBOL_GPL(vmbus_alloc_ring);
+/**
+ * request_arr_init - Allocates memory for the requestor array. Each slot
+ * keeps track of the next available slot in the array. Initially, each
+ * slot points to the next one (as in a Linked List). The last slot
+ * does not point to anything, so its value is U64_MAX by default.
+ * @size The size of the array
+ */
+static u64 *request_arr_init(u32 size)
+{
+ int i;
+ u64 *req_arr;
+
+ req_arr = kcalloc(size, sizeof(u64), GFP_KERNEL);
+ if (!req_arr)
+ return NULL;
+
+ for (i = 0; i < size - 1; i++)
+ req_arr[i] = i + 1;
+
+ /* Last slot (no more available slots) */
+ req_arr[i] = U64_MAX;
+
+ return req_arr;
+}
+
+/*
+ * vmbus_alloc_requestor - Initializes @rqstor's fields.
+ * Slot at index 0 is the first free slot.
+ * @size: Size of the requestor array
+ */
+static int vmbus_alloc_requestor(struct vmbus_requestor *rqstor, u32 size)
+{
+ u64 *rqst_arr;
+ unsigned long *bitmap;
+
+ rqst_arr = request_arr_init(size);
+ if (!rqst_arr)
+ return -ENOMEM;
+
+ bitmap = bitmap_zalloc(size, GFP_KERNEL);
+ if (!bitmap) {
+ kfree(rqst_arr);
+ return -ENOMEM;
+ }
+
+ rqstor->req_arr = rqst_arr;
+ rqstor->req_bitmap = bitmap;
+ rqstor->size = size;
+ rqstor->next_request_id = 0;
+ spin_lock_init(&rqstor->req_lock);
+
+ return 0;
+}
+
+/*
+ * vmbus_free_requestor - Frees memory allocated for @rqstor
+ * @rqstor: Pointer to the requestor struct
+ */
+static void vmbus_free_requestor(struct vmbus_requestor *rqstor)
+{
+ kfree(rqstor->req_arr);
+ bitmap_free(rqstor->req_bitmap);
+}
+
static int __vmbus_open(struct vmbus_channel *newchannel,
void *userdata, u32 userdatalen,
void (*onchannelcallback)(void *context), void *context)
@@ -122,6 +186,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
u32 send_pages, recv_pages;
unsigned long flags;
int err;
+ int rqstor;
if (userdatalen > MAX_USER_DEFINED_BYTES)
return -EINVAL;
@@ -132,6 +197,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
if (newchannel->state != CHANNEL_OPEN_STATE)
return -EINVAL;
+ /* Create and init requestor */
+ if (newchannel->rqstor_size) {
+ rqstor = vmbus_alloc_requestor(&newchannel->requestor,
+ newchannel->rqstor_size);
+ if (rqstor)
+ return -ENOMEM;
+ }
+
newchannel->state = CHANNEL_OPENING_STATE;
newchannel->onchannel_callback = onchannelcallback;
newchannel->channel_callback_context = context;
@@ -228,6 +301,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
error_clean_ring:
hv_ringbuffer_cleanup(&newchannel->outbound);
hv_ringbuffer_cleanup(&newchannel->inbound);
+ vmbus_free_requestor(&newchannel->requestor);
newchannel->state = CHANNEL_OPEN_STATE;
return err;
}
@@ -703,6 +777,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
channel->ringbuffer_gpadlhandle = 0;
}
+ if (!ret)
+ vmbus_free_requestor(&channel->requestor);
+
return ret;
}
@@ -937,3 +1014,75 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
buffer_actual_len, requestid, true);
}
EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
+
+/*
+ * vmbus_next_request_id - Returns a new request id. It is also
+ * the index at which the guest memory address is stored.
+ * Uses a spin lock to avoid race conditions.
+ * @rqstor: Pointer to the requestor struct
+ * @rqst_add: Guest memory address to be stored in the array
+ */
+u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
+{
+ unsigned long flags;
+ u64 current_id;
+
+ spin_lock_irqsave(&rqstor->req_lock, flags);
+ current_id = rqstor->next_request_id;
+
+ /* Requestor array is full */
+ if (current_id >= rqstor->size) {
+ current_id = VMBUS_RQST_ERROR;
+ goto exit;
+ }
+
+ rqstor->next_request_id = rqstor->req_arr[current_id];
+ rqstor->req_arr[current_id] = rqst_addr;
+
+ /* The already held spin lock provides atomicity */
+ bitmap_set(rqstor->req_bitmap, current_id, 1);
+
+exit:
+ spin_unlock_irqrestore(&rqstor->req_lock, flags);
+ return current_id;
+}
+EXPORT_SYMBOL_GPL(vmbus_next_request_id);
+
+/*
+ * vmbus_request_addr - Returns the memory address stored at @trans_id
+ * in @rqstor. Uses a spin lock to avoid race conditions.
+ * @rqstor: Pointer to the requestor struct
+ * @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
+ * next request id.
+ */
+u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
+{
+ unsigned long flags;
+ u64 req_addr;
+
+ spin_lock_irqsave(&rqstor->req_lock, flags);
+
+ /* Invalid trans_id */
+ if (trans_id >= rqstor->size) {
+ req_addr = VMBUS_RQST_ERROR;
+ goto exit;
+ }
+
+ /* Invalid trans_id: empty slot */
+ if (!test_bit(trans_id, rqstor->req_bitmap)) {
+ req_addr = VMBUS_RQST_ERROR;
+ goto exit;
+ }
+
+ req_addr = rqstor->req_arr[trans_id];
+ rqstor->req_arr[trans_id] = rqstor->next_request_id;
+ rqstor->next_request_id = trans_id;
+
+ /* The already held spin lock provides atomicity */
+ bitmap_clear(rqstor->req_bitmap, trans_id, 1);
+
+exit:
+ spin_unlock_irqrestore(&rqstor->req_lock, flags);
+ return req_addr;
+}
+EXPORT_SYMBOL_GPL(vmbus_request_addr);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 38100e80360a..c509d20ab7db 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -716,6 +716,21 @@ enum vmbus_device_type {
HV_UNKNOWN,
};
+/*
+ * Provides request ids for VMBus. Encapsulates guest memory
+ * addresses and stores the next available slot in req_arr
+ * to generate new ids in constant time.
+ */
+struct vmbus_requestor {
+ u64 *req_arr;
+ unsigned long *req_bitmap; /* is a given slot available? */
+ u32 size;
+ u64 next_request_id;
+ spinlock_t req_lock; /* provides atomicity */
+};
+
+#define VMBUS_RQST_ERROR U64_MAX
+
struct vmbus_device {
u16 dev_type;
guid_t guid;
@@ -940,8 +955,14 @@ struct vmbus_channel {
u32 fuzz_testing_interrupt_delay;
u32 fuzz_testing_message_delay;
+ /* request/transaction ids for VMBus */
+ struct vmbus_requestor requestor;
+ u32 rqstor_size;
};
+u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr);
+u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id);
+
static inline bool is_hvsock_channel(const struct vmbus_channel *c)
{
return !!(c->offermsg.offer.chn_flags &
--
2.25.1
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" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
Signed-off-by: Andres Beltran <[email protected]>
---
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
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" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: [email protected]
Signed-off-by: Andres Beltran <[email protected]>
---
drivers/net/hyperv/hyperv_net.h | 10 +++++
drivers/net/hyperv/netvsc.c | 75 +++++++++++++++++++++++++------
drivers/net/hyperv/rndis_filter.c | 1 +
include/linux/hyperv.h | 1 +
4 files changed, 73 insertions(+), 14 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index abda736e7c7d..14735c98e798 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -847,6 +847,16 @@ struct nvsp_message {
#define NETVSC_XDP_HDRM 256
+#define NETVSC_MIN_OUT_MSG_SIZE (sizeof(struct vmpacket_descriptor) + \
+ sizeof(struct nvsp_message))
+#define NETVSC_MIN_IN_MSG_SIZE sizeof(struct vmpacket_descriptor)
+
+/* Estimated requestor size:
+ * out_ring_size/min_out_msg_size + in_ring_size/min_in_msg_size
+ */
+#define NETVSC_RQSTOR_SIZE (netvsc_ring_bytes / NETVSC_MIN_OUT_MSG_SIZE + \
+ netvsc_ring_bytes / NETVSC_MIN_IN_MSG_SIZE)
+
struct multi_send_data {
struct sk_buff *skb; /* skb containing the pkt */
struct hv_netvsc_packet *pkt; /* netvsc pkt pending */
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 41f5cf0bb997..c73d5aef4436 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -50,7 +50,7 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
vmbus_sendpacket(dev->channel, init_pkt,
sizeof(struct nvsp_message),
- (unsigned long)init_pkt,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
}
@@ -163,7 +163,7 @@ static void netvsc_revoke_recv_buf(struct hv_device *device,
ret = vmbus_sendpacket(device->channel,
revoke_packet,
sizeof(struct nvsp_message),
- (unsigned long)revoke_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
/* If the failure is because the channel is rescinded;
* ignore the failure since we cannot send on a rescinded
@@ -213,7 +213,7 @@ static void netvsc_revoke_send_buf(struct hv_device *device,
ret = vmbus_sendpacket(device->channel,
revoke_packet,
sizeof(struct nvsp_message),
- (unsigned long)revoke_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
/* If the failure is because the channel is rescinded;
@@ -304,6 +304,7 @@ static int netvsc_init_buf(struct hv_device *device,
unsigned int buf_size;
size_t map_words;
int ret = 0;
+ u64 rqst_id;
/* Get receive buffer area. */
buf_size = device_info->recv_sections * device_info->recv_section_size;
@@ -350,13 +351,21 @@ static int netvsc_init_buf(struct hv_device *device,
trace_nvsp_send(ndev, init_packet);
+ rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+ netdev_err(ndev, "No request id available\n");
+ goto cleanup;
+ }
+
/* Send the gpadl notification request */
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ rqst_id,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0) {
+ /* Reclaim request ID to avoid leak of IDs */
+ vmbus_request_addr(&device->channel->requestor, rqst_id);
netdev_err(ndev,
"unable to send receive buffer's gpadl to netvsp\n");
goto cleanup;
@@ -432,13 +441,21 @@ static int netvsc_init_buf(struct hv_device *device,
trace_nvsp_send(ndev, init_packet);
+ rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+ netdev_err(ndev, "No request id available\n");
+ goto cleanup;
+ }
+
/* Send the gpadl notification request */
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ rqst_id,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0) {
+ /* Reclaim request ID to avoid leak of IDs */
+ vmbus_request_addr(&device->channel->requestor, rqst_id);
netdev_err(ndev,
"unable to send send buffer's gpadl to netvsp\n");
goto cleanup;
@@ -496,6 +513,7 @@ static int negotiate_nvsp_ver(struct hv_device *device,
{
struct net_device *ndev = hv_get_drvdata(device);
int ret;
+ u64 rqst_id;
memset(init_packet, 0, sizeof(struct nvsp_message));
init_packet->hdr.msg_type = NVSP_MSG_TYPE_INIT;
@@ -503,15 +521,24 @@ static int negotiate_nvsp_ver(struct hv_device *device,
init_packet->msg.init_msg.init.max_protocol_ver = nvsp_ver;
trace_nvsp_send(ndev, init_packet);
+ rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+ netdev_err(ndev, "No request id available\n");
+ return -EAGAIN;
+ }
+
/* Send the init request */
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ rqst_id,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (ret != 0)
+ if (ret != 0) {
+ /* Reclaim request ID to avoid leak of IDs */
+ vmbus_request_addr(&device->channel->requestor, rqst_id);
return ret;
+ }
wait_for_completion(&net_device->channel_init_wait);
@@ -542,7 +569,7 @@ static int negotiate_nvsp_ver(struct hv_device *device,
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
return ret;
@@ -599,7 +626,7 @@ static int netvsc_connect_vsp(struct hv_device *device,
/* Send the init request */
ret = vmbus_sendpacket(device->channel, init_packet,
sizeof(struct nvsp_message),
- (unsigned long)init_packet,
+ VMBUS_RQST_ID_NO_RESPONSE,
VM_PKT_DATA_INBAND, 0);
if (ret != 0)
goto cleanup;
@@ -680,10 +707,19 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
const struct vmpacket_descriptor *desc,
int budget)
{
- struct sk_buff *skb = (struct sk_buff *)(unsigned long)desc->trans_id;
+ struct sk_buff *skb;
struct net_device_context *ndev_ctx = netdev_priv(ndev);
u16 q_idx = 0;
int queue_sends;
+ u64 cmd_rqst;
+
+ cmd_rqst = vmbus_request_addr(&channel->requestor, (u64)desc->trans_id);
+ if (cmd_rqst == VMBUS_RQST_ERROR) {
+ netdev_err(ndev, "Incorrect transaction id\n");
+ return;
+ }
+
+ skb = (struct sk_buff *)cmd_rqst;
/* Notify the layer above us */
if (likely(skb)) {
@@ -822,7 +858,7 @@ static inline int netvsc_send_pkt(
struct net_device *ndev = hv_get_drvdata(device);
struct net_device_context *ndev_ctx = netdev_priv(ndev);
struct netdev_queue *txq = netdev_get_tx_queue(ndev, packet->q_idx);
- u64 req_id;
+ u64 rqst_id;
int ret;
u32 ring_avail = hv_get_avail_to_write_percent(&out_channel->outbound);
@@ -838,13 +874,18 @@ static inline int netvsc_send_pkt(
else
rpkt->send_buf_section_size = packet->total_data_buflen;
- req_id = (ulong)skb;
if (out_channel->rescind)
return -ENODEV;
trace_nvsp_send_pkt(ndev, out_channel, rpkt);
+ rqst_id = vmbus_next_request_id(&out_channel->requestor, (u64)skb);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+ ret = -EAGAIN;
+ goto ret_check;
+ }
+
if (packet->page_buf_cnt) {
if (packet->cp_partial)
pb += packet->rmsg_pgcnt;
@@ -852,14 +893,15 @@ static inline int netvsc_send_pkt(
ret = vmbus_sendpacket_pagebuffer(out_channel,
pb, packet->page_buf_cnt,
&nvmsg, sizeof(nvmsg),
- req_id);
+ rqst_id);
} else {
ret = vmbus_sendpacket(out_channel,
&nvmsg, sizeof(nvmsg),
- req_id, VM_PKT_DATA_INBAND,
+ rqst_id, VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
}
+ret_check:
if (ret == 0) {
atomic_inc_return(&nvchan->queue_sends);
@@ -868,9 +910,13 @@ static inline int netvsc_send_pkt(
ndev_ctx->eth_stats.stop_queue++;
}
} else if (ret == -EAGAIN) {
+ /* Reclaim request ID to avoid leak of IDs */
+ vmbus_request_addr(&out_channel->requestor, rqst_id);
netif_tx_stop_queue(txq);
ndev_ctx->eth_stats.stop_queue++;
} else {
+ /* Reclaim request ID to avoid leak of IDs */
+ vmbus_request_addr(&out_channel->requestor, rqst_id);
netdev_err(ndev,
"Unable to send packet pages %u len %u, ret %d\n",
packet->page_buf_cnt, packet->total_data_buflen,
@@ -1422,6 +1468,7 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
netvsc_poll, NAPI_POLL_WEIGHT);
/* Open the channel */
+ device->channel->rqstor_size = NETVSC_RQSTOR_SIZE;
ret = vmbus_open(device->channel, netvsc_ring_bytes,
netvsc_ring_bytes, NULL, 0,
netvsc_channel_cb, net_device->chan_table);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index b81ceba38218..4d96e8e5ea24 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -1114,6 +1114,7 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
/* Set the channel before opening.*/
nvchan->channel = new_sc;
+ new_sc->rqstor_size = NETVSC_RQSTOR_SIZE;
ret = vmbus_open(new_sc, netvsc_ring_bytes,
netvsc_ring_bytes, NULL, 0,
netvsc_channel_cb, nvchan);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index c509d20ab7db..d8194924983d 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -730,6 +730,7 @@ struct vmbus_requestor {
};
#define VMBUS_RQST_ERROR U64_MAX
+#define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 1)
struct vmbus_device {
u16 dev_type;
--
2.25.1
> Andres Beltran (3):
> Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
> hardening
> scsi: storvsc: Use vmbus_requestor to generate transaction ids for
> VMBus hardening
> hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
> hardening
For the series,
Tested-by: Andrea Parri <[email protected]>
Thanks,
Andrea
> -----Original Message-----
> From: Andres Beltran <[email protected]>
> Sent: Thursday, June 25, 2020 11:37 AM
> To: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Stephen Hemminger <[email protected]>;
> [email protected]
> Cc: [email protected]; [email protected]; Michael
> Kelley <[email protected]>; [email protected]; Andres Beltran
> <[email protected]>; David S. Miller <[email protected]>; Jakub
> Kicinski <[email protected]>; [email protected]
> Subject: [PATCH 3/3] hv_netvsc: Use vmbus_requestor to generate transaction
> IDs for VMBus hardening
>
> Currently, pointers to guest memory are passed to Hyper-V as
> transaction IDs in netvsc. In the face of errors or malicious
> behavior in Hyper-V, netvsc should not expose or trust the transaction
> IDs returned by Hyper-V to be valid guest memory addresses. Instead,
> use small integers generated by vmbus_requestor as requests
> (transaction) IDs.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andres Beltran <[email protected]>
> ---
> drivers/net/hyperv/hyperv_net.h | 10 +++++
> drivers/net/hyperv/netvsc.c | 75 +++++++++++++++++++++++++------
> drivers/net/hyperv/rndis_filter.c | 1 +
> include/linux/hyperv.h | 1 +
> 4 files changed, 73 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index abda736e7c7d..14735c98e798 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -847,6 +847,16 @@ struct nvsp_message {
>
> #define NETVSC_XDP_HDRM 256
>
> +#define NETVSC_MIN_OUT_MSG_SIZE (sizeof(struct vmpacket_descriptor) + \
> + sizeof(struct nvsp_message))
> +#define NETVSC_MIN_IN_MSG_SIZE sizeof(struct vmpacket_descriptor)
> +
> +/* Estimated requestor size:
> + * out_ring_size/min_out_msg_size + in_ring_size/min_in_msg_size
> + */
> +#define NETVSC_RQSTOR_SIZE (netvsc_ring_bytes /
> NETVSC_MIN_OUT_MSG_SIZE + \
> + netvsc_ring_bytes / NETVSC_MIN_IN_MSG_SIZE)
Please make the variable as the macro parameter for readability:
#define NETVSC_RQSTOR_SIZE(ringbytes) (ringbytes / NETVSC_MIN_OUT_MSG_SIZE ...
Then put the actual variable name when you use the macro.
Thanks,
- Haiyang
Hi Andres,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20200625]
[also build test WARNING on v5.8-rc2]
[cannot apply to mkp-scsi/for-next scsi/for-next linux/master linus/master v5.8-rc2 v5.8-rc1 v5.7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andres-Beltran/Drivers-hv-vmbus-vmbus_requestor-data-structure/20200625-234113
base: 3f9437c6234d95d96967f1b438a4fb71b6be254d
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/net/hyperv/netvsc.c: In function 'netvsc_init_buf':
>> drivers/net/hyperv/netvsc.c:354:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
354 | rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
| ^
drivers/net/hyperv/netvsc.c:444:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
444 | rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
| ^
drivers/net/hyperv/netvsc.c: In function 'negotiate_nvsp_ver':
drivers/net/hyperv/netvsc.c:524:63: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
524 | rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
| ^
drivers/net/hyperv/netvsc.c: In function 'netvsc_send_tx_complete':
>> drivers/net/hyperv/netvsc.c:722:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
722 | skb = (struct sk_buff *)cmd_rqst;
| ^
drivers/net/hyperv/netvsc.c: In function 'netvsc_send_pkt':
drivers/net/hyperv/netvsc.c:883:59: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
883 | rqst_id = vmbus_next_request_id(&out_channel->requestor, (u64)skb);
| ^
vim +354 drivers/net/hyperv/netvsc.c
296
297 static int netvsc_init_buf(struct hv_device *device,
298 struct netvsc_device *net_device,
299 const struct netvsc_device_info *device_info)
300 {
301 struct nvsp_1_message_send_receive_buffer_complete *resp;
302 struct net_device *ndev = hv_get_drvdata(device);
303 struct nvsp_message *init_packet;
304 unsigned int buf_size;
305 size_t map_words;
306 int ret = 0;
307 u64 rqst_id;
308
309 /* Get receive buffer area. */
310 buf_size = device_info->recv_sections * device_info->recv_section_size;
311 buf_size = roundup(buf_size, PAGE_SIZE);
312
313 /* Legacy hosts only allow smaller receive buffer */
314 if (net_device->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
315 buf_size = min_t(unsigned int, buf_size,
316 NETVSC_RECEIVE_BUFFER_SIZE_LEGACY);
317
318 net_device->recv_buf = vzalloc(buf_size);
319 if (!net_device->recv_buf) {
320 netdev_err(ndev,
321 "unable to allocate receive buffer of size %u\n",
322 buf_size);
323 ret = -ENOMEM;
324 goto cleanup;
325 }
326
327 net_device->recv_buf_size = buf_size;
328
329 /*
330 * Establish the gpadl handle for this buffer on this
331 * channel. Note: This call uses the vmbus connection rather
332 * than the channel to establish the gpadl handle.
333 */
334 ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
335 buf_size,
336 &net_device->recv_buf_gpadl_handle);
337 if (ret != 0) {
338 netdev_err(ndev,
339 "unable to establish receive buffer's gpadl\n");
340 goto cleanup;
341 }
342
343 /* Notify the NetVsp of the gpadl handle */
344 init_packet = &net_device->channel_init_pkt;
345 memset(init_packet, 0, sizeof(struct nvsp_message));
346 init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_RECV_BUF;
347 init_packet->msg.v1_msg.send_recv_buf.
348 gpadl_handle = net_device->recv_buf_gpadl_handle;
349 init_packet->msg.v1_msg.
350 send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
351
352 trace_nvsp_send(ndev, init_packet);
353
> 354 rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
355 if (rqst_id == VMBUS_RQST_ERROR) {
356 netdev_err(ndev, "No request id available\n");
357 goto cleanup;
358 }
359
360 /* Send the gpadl notification request */
361 ret = vmbus_sendpacket(device->channel, init_packet,
362 sizeof(struct nvsp_message),
363 rqst_id,
364 VM_PKT_DATA_INBAND,
365 VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
366 if (ret != 0) {
367 /* Reclaim request ID to avoid leak of IDs */
368 vmbus_request_addr(&device->channel->requestor, rqst_id);
369 netdev_err(ndev,
370 "unable to send receive buffer's gpadl to netvsp\n");
371 goto cleanup;
372 }
373
374 wait_for_completion(&net_device->channel_init_wait);
375
376 /* Check the response */
377 resp = &init_packet->msg.v1_msg.send_recv_buf_complete;
378 if (resp->status != NVSP_STAT_SUCCESS) {
379 netdev_err(ndev,
380 "Unable to complete receive buffer initialization with NetVsp - status %d\n",
381 resp->status);
382 ret = -EINVAL;
383 goto cleanup;
384 }
385
386 /* Parse the response */
387 netdev_dbg(ndev, "Receive sections: %u sub_allocs: size %u count: %u\n",
388 resp->num_sections, resp->sections[0].sub_alloc_size,
389 resp->sections[0].num_sub_allocs);
390
391 /* There should only be one section for the entire receive buffer */
392 if (resp->num_sections != 1 || resp->sections[0].offset != 0) {
393 ret = -EINVAL;
394 goto cleanup;
395 }
396
397 net_device->recv_section_size = resp->sections[0].sub_alloc_size;
398 net_device->recv_section_cnt = resp->sections[0].num_sub_allocs;
399
400 /* Setup receive completion ring.
401 * Add 1 to the recv_section_cnt because at least one entry in a
402 * ring buffer has to be empty.
403 */
404 net_device->recv_completion_cnt = net_device->recv_section_cnt + 1;
405 ret = netvsc_alloc_recv_comp_ring(net_device, 0);
406 if (ret)
407 goto cleanup;
408
409 /* Now setup the send buffer. */
410 buf_size = device_info->send_sections * device_info->send_section_size;
411 buf_size = round_up(buf_size, PAGE_SIZE);
412
413 net_device->send_buf = vzalloc(buf_size);
414 if (!net_device->send_buf) {
415 netdev_err(ndev, "unable to allocate send buffer of size %u\n",
416 buf_size);
417 ret = -ENOMEM;
418 goto cleanup;
419 }
420
421 /* Establish the gpadl handle for this buffer on this
422 * channel. Note: This call uses the vmbus connection rather
423 * than the channel to establish the gpadl handle.
424 */
425 ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
426 buf_size,
427 &net_device->send_buf_gpadl_handle);
428 if (ret != 0) {
429 netdev_err(ndev,
430 "unable to establish send buffer's gpadl\n");
431 goto cleanup;
432 }
433
434 /* Notify the NetVsp of the gpadl handle */
435 init_packet = &net_device->channel_init_pkt;
436 memset(init_packet, 0, sizeof(struct nvsp_message));
437 init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
438 init_packet->msg.v1_msg.send_send_buf.gpadl_handle =
439 net_device->send_buf_gpadl_handle;
440 init_packet->msg.v1_msg.send_send_buf.id = NETVSC_SEND_BUFFER_ID;
441
442 trace_nvsp_send(ndev, init_packet);
443
444 rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)init_packet);
445 if (rqst_id == VMBUS_RQST_ERROR) {
446 netdev_err(ndev, "No request id available\n");
447 goto cleanup;
448 }
449
450 /* Send the gpadl notification request */
451 ret = vmbus_sendpacket(device->channel, init_packet,
452 sizeof(struct nvsp_message),
453 rqst_id,
454 VM_PKT_DATA_INBAND,
455 VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
456 if (ret != 0) {
457 /* Reclaim request ID to avoid leak of IDs */
458 vmbus_request_addr(&device->channel->requestor, rqst_id);
459 netdev_err(ndev,
460 "unable to send send buffer's gpadl to netvsp\n");
461 goto cleanup;
462 }
463
464 wait_for_completion(&net_device->channel_init_wait);
465
466 /* Check the response */
467 if (init_packet->msg.v1_msg.
468 send_send_buf_complete.status != NVSP_STAT_SUCCESS) {
469 netdev_err(ndev, "Unable to complete send buffer "
470 "initialization with NetVsp - status %d\n",
471 init_packet->msg.v1_msg.
472 send_send_buf_complete.status);
473 ret = -EINVAL;
474 goto cleanup;
475 }
476
477 /* Parse the response */
478 net_device->send_section_size = init_packet->msg.
479 v1_msg.send_send_buf_complete.section_size;
480
481 /* Section count is simply the size divided by the section size. */
482 net_device->send_section_cnt = buf_size / net_device->send_section_size;
483
484 netdev_dbg(ndev, "Send section size: %d, Section count:%d\n",
485 net_device->send_section_size, net_device->send_section_cnt);
486
487 /* Setup state for managing the send buffer. */
488 map_words = DIV_ROUND_UP(net_device->send_section_cnt, BITS_PER_LONG);
489
490 net_device->send_section_map = kcalloc(map_words, sizeof(ulong), GFP_KERNEL);
491 if (net_device->send_section_map == NULL) {
492 ret = -ENOMEM;
493 goto cleanup;
494 }
495
496 goto exit;
497
498 cleanup:
499 netvsc_revoke_recv_buf(device, net_device, ndev);
500 netvsc_revoke_send_buf(device, net_device, ndev);
501 netvsc_teardown_recv_gpadl(device, net_device, ndev);
502 netvsc_teardown_send_gpadl(device, net_device, ndev);
503
504 exit:
505 return ret;
506 }
507
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, Jun 25, 2020 at 11:37:21AM -0400, Andres Beltran wrote:
> Currently, VMbus drivers use pointers into guest memory as request IDs
> for interactions with Hyper-V. To be more robust in the face of errors
> or malicious behavior from a compromised Hyper-V, avoid exposing
> guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> bad request ID that is then treated as the address of a guest data
> structure with no validation. Instead, encapsulate these memory
> addresses and provide small integers as request IDs.
>
> Signed-off-by: Andres Beltran <[email protected]>
> ---
> drivers/hv/channel.c | 149 +++++++++++++++++++++++++++++++++++++++++
> include/linux/hyperv.h | 21 ++++++
> 2 files changed, 170 insertions(+)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 3ebda7707e46..2ea1bfecbfda 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -112,6 +112,70 @@ int vmbus_alloc_ring(struct vmbus_channel *newchannel,
> }
> EXPORT_SYMBOL_GPL(vmbus_alloc_ring);
>
> +/**
> + * request_arr_init - Allocates memory for the requestor array. Each slot
> + * keeps track of the next available slot in the array. Initially, each
> + * slot points to the next one (as in a Linked List). The last slot
> + * does not point to anything, so its value is U64_MAX by default.
> + * @size The size of the array
> + */
> +static u64 *request_arr_init(u32 size)
> +{
> + int i;
> + u64 *req_arr;
> +
> + req_arr = kcalloc(size, sizeof(u64), GFP_KERNEL);
> + if (!req_arr)
> + return NULL;
> +
> + for (i = 0; i < size - 1; i++)
> + req_arr[i] = i + 1;
> +
> + /* Last slot (no more available slots) */
> + req_arr[i] = U64_MAX;
> +
> + return req_arr;
> +}
> +
> +/*
> + * vmbus_alloc_requestor - Initializes @rqstor's fields.
> + * Slot at index 0 is the first free slot.
> + * @size: Size of the requestor array
> + */
> +static int vmbus_alloc_requestor(struct vmbus_requestor *rqstor, u32 size)
> +{
> + u64 *rqst_arr;
> + unsigned long *bitmap;
> +
> + rqst_arr = request_arr_init(size);
> + if (!rqst_arr)
> + return -ENOMEM;
> +
> + bitmap = bitmap_zalloc(size, GFP_KERNEL);
> + if (!bitmap) {
> + kfree(rqst_arr);
> + return -ENOMEM;
> + }
> +
> + rqstor->req_arr = rqst_arr;
> + rqstor->req_bitmap = bitmap;
> + rqstor->size = size;
> + rqstor->next_request_id = 0;
> + spin_lock_init(&rqstor->req_lock);
> +
> + return 0;
> +}
> +
> +/*
> + * vmbus_free_requestor - Frees memory allocated for @rqstor
> + * @rqstor: Pointer to the requestor struct
> + */
> +static void vmbus_free_requestor(struct vmbus_requestor *rqstor)
> +{
> + kfree(rqstor->req_arr);
> + bitmap_free(rqstor->req_bitmap);
> +}
> +
> static int __vmbus_open(struct vmbus_channel *newchannel,
> void *userdata, u32 userdatalen,
> void (*onchannelcallback)(void *context), void *context)
> @@ -122,6 +186,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> u32 send_pages, recv_pages;
> unsigned long flags;
> int err;
> + int rqstor;
>
> if (userdatalen > MAX_USER_DEFINED_BYTES)
> return -EINVAL;
> @@ -132,6 +197,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> if (newchannel->state != CHANNEL_OPEN_STATE)
> return -EINVAL;
>
> + /* Create and init requestor */
> + if (newchannel->rqstor_size) {
> + rqstor = vmbus_alloc_requestor(&newchannel->requestor,
> + newchannel->rqstor_size);
You can simply use err here to store the return value or even get rid of
rqstor by doing
if (vmbus_alloc_requestor(...))
> + if (rqstor)
> + return -ENOMEM;
> + }
> +
> newchannel->state = CHANNEL_OPENING_STATE;
> newchannel->onchannel_callback = onchannelcallback;
> newchannel->channel_callback_context = context;
> @@ -228,6 +301,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> error_clean_ring:
> hv_ringbuffer_cleanup(&newchannel->outbound);
> hv_ringbuffer_cleanup(&newchannel->inbound);
> + vmbus_free_requestor(&newchannel->requestor);
> newchannel->state = CHANNEL_OPEN_STATE;
> return err;
> }
> @@ -703,6 +777,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
> channel->ringbuffer_gpadlhandle = 0;
> }
>
> + if (!ret)
> + vmbus_free_requestor(&channel->requestor);
> +
> return ret;
> }
>
> @@ -937,3 +1014,75 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
> buffer_actual_len, requestid, true);
> }
> EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> +
> +/*
> + * vmbus_next_request_id - Returns a new request id. It is also
> + * the index at which the guest memory address is stored.
> + * Uses a spin lock to avoid race conditions.
> + * @rqstor: Pointer to the requestor struct
> + * @rqst_add: Guest memory address to be stored in the array
> + */
> +u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> +{
> + unsigned long flags;
> + u64 current_id;
> +
> + spin_lock_irqsave(&rqstor->req_lock, flags);
Do you really need the irqsave variant here? I.e. is there really a
chance this code is reachable from an interrupt handler?
Wei.
On Thu, Jun 25, 2020 at 11:37:22AM -0400, Andres Beltran wrote:
> * If the number of CPUs is artificially restricted, such as
> @@ -760,14 +768,22 @@ static void handle_multichannel_storage(struct hv_device *device, int max_chns)
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> vstor_packet->sub_channel_count = num_sc;
>
> + rqst_id = vmbus_next_request_id(&device->channel->requestor, (u64)request);
You can cast request to unsigned long to fix the warnings on 32bit.
Wei.
On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> From: Andres Beltran (Microsoft) <[email protected]>
>
> Currently, VMbus drivers use pointers into guest memory as request IDs
> for interactions with Hyper-V. To be more robust in the face of errors
> or malicious behavior from a compromised Hyper-V, avoid exposing
> guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> bad request ID that is then treated as the address of a guest data
> structure with no validation. Instead, encapsulate these memory
> addresses and provide small integers as request IDs.
>
> The first patch creates the definitions for the data structure, provides
> helper methods to generate new IDs and retrieve data, and
> allocates/frees the memory needed for vmbus_requestor.
>
> The second and third patches make use of vmbus_requestor to send request
> IDs to Hyper-V in storvsc and netvsc respectively.
>
Per my understanding, this new data structure is per-channel, so it
won't introduce contention on the lock in multi-queue scenario. Have you
done any testing to confirm there is no severe performance regression?
Wei.
> Thanks.
> Andres Beltran
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: "James E.J. Bottomley" <[email protected]>
> Cc: "Martin K. Petersen" <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
>
> Andres Beltran (3):
> Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus
> hardening
> scsi: storvsc: Use vmbus_requestor to generate transaction ids for
> VMBus hardening
> hv_netvsc: Use vmbus_requestor to generate transaction ids for VMBus
> hardening
>
> drivers/hv/channel.c | 150 ++++++++++++++++++++++++++++++
> drivers/net/hyperv/hyperv_net.h | 10 ++
> drivers/net/hyperv/netvsc.c | 56 +++++++++--
> drivers/net/hyperv/rndis_filter.c | 1 +
> drivers/scsi/storvsc_drv.c | 62 ++++++++++--
> include/linux/hyperv.h | 22 +++++
> 6 files changed, 283 insertions(+), 18 deletions(-)
>
> --
> 2.25.1
>
On Fri, Jun 26, 2020 at 01:42:27PM +0000, Wei Liu wrote:
> On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> > From: Andres Beltran (Microsoft) <[email protected]>
> >
> > Currently, VMbus drivers use pointers into guest memory as request IDs
> > for interactions with Hyper-V. To be more robust in the face of errors
> > or malicious behavior from a compromised Hyper-V, avoid exposing
> > guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> > bad request ID that is then treated as the address of a guest data
> > structure with no validation. Instead, encapsulate these memory
> > addresses and provide small integers as request IDs.
> >
> > The first patch creates the definitions for the data structure, provides
> > helper methods to generate new IDs and retrieve data, and
> > allocates/frees the memory needed for vmbus_requestor.
> >
> > The second and third patches make use of vmbus_requestor to send request
> > IDs to Hyper-V in storvsc and netvsc respectively.
> >
>
> Per my understanding, this new data structure is per-channel, so it
> won't introduce contention on the lock in multi-queue scenario. Have you
> done any testing to confirm there is no severe performance regression?
I did run some performance tests using our dev pipeline (storage and
network workloads). I did not find regressions w.r.t. baseline.
Andrea
On Fri, Jun 26, 2020 at 04:48:17PM +0200, Andrea Parri wrote:
> On Fri, Jun 26, 2020 at 01:42:27PM +0000, Wei Liu wrote:
> > On Thu, Jun 25, 2020 at 11:37:20AM -0400, Andres Beltran wrote:
> > > From: Andres Beltran (Microsoft) <[email protected]>
> > >
> > > Currently, VMbus drivers use pointers into guest memory as request IDs
> > > for interactions with Hyper-V. To be more robust in the face of errors
> > > or malicious behavior from a compromised Hyper-V, avoid exposing
> > > guest memory addresses to Hyper-V. Also avoid Hyper-V giving back a
> > > bad request ID that is then treated as the address of a guest data
> > > structure with no validation. Instead, encapsulate these memory
> > > addresses and provide small integers as request IDs.
> > >
> > > The first patch creates the definitions for the data structure, provides
> > > helper methods to generate new IDs and retrieve data, and
> > > allocates/frees the memory needed for vmbus_requestor.
> > >
> > > The second and third patches make use of vmbus_requestor to send request
> > > IDs to Hyper-V in storvsc and netvsc respectively.
> > >
> >
> > Per my understanding, this new data structure is per-channel, so it
> > won't introduce contention on the lock in multi-queue scenario. Have you
> > done any testing to confirm there is no severe performance regression?
>
> I did run some performance tests using our dev pipeline (storage and
> network workloads). I did not find regressions w.r.t. baseline.
Thanks, that's good to hear.
Wei.
>
> Andrea
On Mon, Jun 29, 2020 at 06:19:46PM +0000, Andres Beltran wrote:
[...]
> > > EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> > > +
> > > +/*
> > > + * vmbus_next_request_id - Returns a new request id. It is also
> > > + * the index at which the guest memory address is stored.
> > > + * Uses a spin lock to avoid race conditions.
> > > + * @rqstor: Pointer to the requestor struct
> > > + * @rqst_add: Guest memory address to be stored in the array
> > > + */
> > > +u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> > > +{
> > > + unsigned long flags;
> > > + u64 current_id;
> > > +
> > > + spin_lock_irqsave(&rqstor->req_lock, flags);
> >
> > Do you really need the irqsave variant here? I.e. is there really a
> > chance this code is reachable from an interrupt handler?
>
> Other VMBus drivers will also need to use this functionality, and
> some of them will be called with interrupts disabled. So, I think
> we should keep the irqsave variant here.
>
Okay. This makes sense.
Wei.
From: [email protected] <[email protected]> On Behalf
Of Wei Liu. Sent: Friday, June 26, 2020 9:20 AM
> > static int __vmbus_open(struct vmbus_channel *newchannel,
> > void *userdata, u32 userdatalen,
> > void (*onchannelcallback)(void *context), void *context)
> > @@ -122,6 +186,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> > u32 send_pages, recv_pages;
> > unsigned long flags;
> > int err;
> > + int rqstor;
> >
> > if (userdatalen > MAX_USER_DEFINED_BYTES)
> > return -EINVAL;
> > @@ -132,6 +197,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> > if (newchannel->state != CHANNEL_OPEN_STATE)
> > return -EINVAL;
> >
> > + /* Create and init requestor */
> > + if (newchannel->rqstor_size) {
> > + rqstor = vmbus_alloc_requestor(&newchannel->requestor,
> > + newchannel->rqstor_size);
>
> You can simply use err here to store the return value or even get rid of
> rqstor by doing
Right. I will do that.
> > @@ -937,3 +1014,75 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void
> *buffer,
> > buffer_actual_len, requestid, true);
> > }
> > EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> > +
> > +/*
> > + * vmbus_next_request_id - Returns a new request id. It is also
> > + * the index at which the guest memory address is stored.
> > + * Uses a spin lock to avoid race conditions.
> > + * @rqstor: Pointer to the requestor struct
> > + * @rqst_add: Guest memory address to be stored in the array
> > + */
> > +u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> > +{
> > + unsigned long flags;
> > + u64 current_id;
> > +
> > + spin_lock_irqsave(&rqstor->req_lock, flags);
>
> Do you really need the irqsave variant here? I.e. is there really a
> chance this code is reachable from an interrupt handler?
Other VMBus drivers will also need to use this functionality, and
some of them will be called with interrupts disabled. So, I think
we should keep the irqsave variant here.
Andres.