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 | 146 ++++++++++++++++++++++++++++++
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, 321 insertions(+), 25 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]>
---
Changes in v2:
- Get rid of "rqstor" variable in __vmbus_open().
drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
include/linux/hyperv.h | 21 ++++++
2 files changed, 167 insertions(+)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 3ebda7707e46..c89d57d0c2d2 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,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
On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> ---
> Changes in v2:
> - Get rid of "rqstor" variable in __vmbus_open().
>
> drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> include/linux/hyperv.h | 21 ++++++
> 2 files changed, 167 insertions(+)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 3ebda7707e46..c89d57d0c2d2 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;
> + }
> +
Sorry for not noticing this in the last round: this infrastructure is
initialized conditionally but used unconditionally.
I can think of two options here:
1. Mandate rqstor_size to be non-zero. Always initialize this
infra.
2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
uninitialized state.
For #2, you can simply check rqstor->size _before_ taking the lock
(because it may be uninitialized, and the assumption is ->size will not
change during the channel's lifetime, hence no lock is needed) and
simply return the same value to the caller.
Wei.
On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > ---
> > Changes in v2:
> > - Get rid of "rqstor" variable in __vmbus_open().
> >
> > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > include/linux/hyperv.h | 21 ++++++
> > 2 files changed, 167 insertions(+)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 3ebda7707e46..c89d57d0c2d2 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;
> > + }
> > +
>
> Sorry for not noticing this in the last round: this infrastructure is
> initialized conditionally but used unconditionally.
>
> I can think of two options here:
>
> 1. Mandate rqstor_size to be non-zero. Always initialize this
> infra.
> 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> uninitialized state.
>
> For #2, you can simply check rqstor->size _before_ taking the lock
> (because it may be uninitialized, and the assumption is ->size will not
> change during the channel's lifetime, hence no lock is needed) and
> simply return the same value to the caller.
>
> Wei.
Right. I think option #2 would be preferable in this case, because #1 works
if we had a default non-zero size for cases where rqstor_size has not been
set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
do you mean by "same value"? I think we would need to return
VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
returning the same guest memory address which we don't want to expose.
Andres.
From: Andres Beltran <[email protected]> Sent: Monday, June 29, 2020 2:51 PM
>
> On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > ---
> > > Changes in v2:
> > > - Get rid of "rqstor" variable in __vmbus_open().
> > >
> > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > include/linux/hyperv.h | 21 ++++++
> > > 2 files changed, 167 insertions(+)
> > >
> > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > + }
> > > +
> >
> > Sorry for not noticing this in the last round: this infrastructure is
> > initialized conditionally but used unconditionally.
> >
> > I can think of two options here:
> >
> > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > infra.
> > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > uninitialized state.
> >
> > For #2, you can simply check rqstor->size _before_ taking the lock
> > (because it may be uninitialized, and the assumption is ->size will not
> > change during the channel's lifetime, hence no lock is needed) and
> > simply return the same value to the caller.
> >
> > Wei.
>
> Right. I think option #2 would be preferable in this case, because #1 works
> if we had a default non-zero size for cases where rqstor_size has not been
> set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> do you mean by "same value"? I think we would need to return
> VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> returning the same guest memory address which we don't want to expose.
>
I'm not understanding the problem here. Any VMbus driver that uses
this requestID allocation mechanism must set newchannel->rqstor_size
to a non-zero value. But if a VMbus driver doesn't use the mechanism,
then newchannel->rqstor_size will default to zero, and the mechanism
will not be initialized for the channels used by that driver. I think the
cleanup of the mechanism handles the case where it wasn't ever
initialized. Or am I missing something?
Michael
On Mon, Jun 29, 2020 at 5:56 PM Michael Kelley <[email protected]> wrote:
> I'm not understanding the problem here. Any VMbus driver that uses
> this requestID allocation mechanism must set newchannel->rqstor_size
> to a non-zero value. But if a VMbus driver doesn't use the mechanism,
> then newchannel->rqstor_size will default to zero, and the mechanism
> will not be initialized for the channels used by that driver. I think the
> cleanup of the mechanism handles the case where it wasn't ever
> initialized. Or am I missing something?
>
> Michael
Yes, that is correct. I think the validation is necessary if there
exists an instance
where a driver call vmbus_next_request_id or vmbus_request_addr() with a rqstor
that has not been previously initialized. Currently, the rqstor
pointer is not being
validated in these 2 functions, because we assume that the driver has
initialized the
array with a non-zero value before calling next_id() or request_addr().
Andres.
On Mon, Jun 29, 2020 at 09:56:08PM +0000, Michael Kelley wrote:
> From: Andres Beltran <[email protected]> Sent: Monday, June 29, 2020 2:51 PM
> >
> > On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > > ---
> > > > Changes in v2:
> > > > - Get rid of "rqstor" variable in __vmbus_open().
> > > >
> > > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/hyperv.h | 21 ++++++
> > > > 2 files changed, 167 insertions(+)
> > > >
> > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > > + }
> > > > +
> > >
> > > Sorry for not noticing this in the last round: this infrastructure is
> > > initialized conditionally but used unconditionally.
> > >
> > > I can think of two options here:
> > >
> > > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > > infra.
> > > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > > uninitialized state.
> > >
> > > For #2, you can simply check rqstor->size _before_ taking the lock
> > > (because it may be uninitialized, and the assumption is ->size will not
> > > change during the channel's lifetime, hence no lock is needed) and
> > > simply return the same value to the caller.
> > >
> > > Wei.
> >
> > Right. I think option #2 would be preferable in this case, because #1 works
> > if we had a default non-zero size for cases where rqstor_size has not been
> > set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> > do you mean by "same value"? I think we would need to return
> > VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> > returning the same guest memory address which we don't want to expose.
> >
>
> I'm not understanding the problem here. Any VMbus driver that uses
> this requestID allocation mechanism must set newchannel->rqstor_size
> to a non-zero value. But if a VMbus driver doesn't use the mechanism,
> then newchannel->rqstor_size will default to zero, and the mechanism
> will not be initialized for the channels used by that driver. I think the
> cleanup of the mechanism handles the case where it wasn't ever
> initialized. Or am I missing something?
>
It is not about the cleanup function -- it handles things correctly
because kfree etc can cope with NULL pointers.
I'm referring to vmbus_next_request_id and vmbus_request_addr. They are
called in later patches regardless of whether the infrastructure is
initialized or not.
That is problematic, because the first thing those functions do is to
acquire the spinlock, which is not guaranteed to be initialized -- it is
initialized in vmbus_alloc_requestor which is called conditionally.
Wei.
> Michael
On Mon, Jun 29, 2020 at 05:51:05PM -0400, Andres Beltran wrote:
> On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > ---
> > > Changes in v2:
> > > - Get rid of "rqstor" variable in __vmbus_open().
> > >
> > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > include/linux/hyperv.h | 21 ++++++
> > > 2 files changed, 167 insertions(+)
> > >
> > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > + }
> > > +
> >
> > Sorry for not noticing this in the last round: this infrastructure is
> > initialized conditionally but used unconditionally.
> >
> > I can think of two options here:
> >
> > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > infra.
> > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > uninitialized state.
> >
> > For #2, you can simply check rqstor->size _before_ taking the lock
> > (because it may be uninitialized, and the assumption is ->size will not
> > change during the channel's lifetime, hence no lock is needed) and
> > simply return the same value to the caller.
> >
> > Wei.
>
> Right. I think option #2 would be preferable in this case, because #1 works
> if we had a default non-zero size for cases where rqstor_size has not been
> set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> do you mean by "same value"? I think we would need to return
> VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> returning the same guest memory address which we don't want to expose.
By "same value", I meant reverting back to using guest memory address.
I thought downgrading gracefully is better than making the driver stop
working.
If exposing guest address is not acceptable, you can return
VMBUS_RQST_ERROR -- but at the point you may as well mandate requestor
infrastructure to be always initialized, right?
Wei.
>
> Andres.
On Mon, Jun 29, 2020 at 6:20 PM Wei Liu <[email protected]> wrote:
>
> On Mon, Jun 29, 2020 at 05:51:05PM -0400, Andres Beltran wrote:
> > On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > > ---
> > > > Changes in v2:
> > > > - Get rid of "rqstor" variable in __vmbus_open().
> > > >
> > > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/hyperv.h | 21 ++++++
> > > > 2 files changed, 167 insertions(+)
> > > >
> > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > > + }
> > > > +
> > >
> > > Sorry for not noticing this in the last round: this infrastructure is
> > > initialized conditionally but used unconditionally.
> > >
> > > I can think of two options here:
> > >
> > > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > > infra.
> > > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > > uninitialized state.
> > >
> > > For #2, you can simply check rqstor->size _before_ taking the lock
> > > (because it may be uninitialized, and the assumption is ->size will not
> > > change during the channel's lifetime, hence no lock is needed) and
> > > simply return the same value to the caller.
> > >
> > > Wei.
> >
> > Right. I think option #2 would be preferable in this case, because #1 works
> > if we had a default non-zero size for cases where rqstor_size has not been
> > set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> > do you mean by "same value"? I think we would need to return
> > VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> > returning the same guest memory address which we don't want to expose.
>
> By "same value", I meant reverting back to using guest memory address.
> I thought downgrading gracefully is better than making the driver stop
> working.
>
> If exposing guest address is not acceptable, you can return
> VMBUS_RQST_ERROR -- but at the point you may as well mandate requestor
> infrastructure to be always initialized, right?
>
If the allocation of the requestor fails during runtime, vmbus_open()
fails too and therefore,
the channel and the requestor will not be created. So, the 2 functions
(next_id, requestor_addr)
will never get called, right? The only case in which we hit this edge
case is if a driver is using this
mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
value before calling vmbus_open()),
but that would be more like a coding bug. So, I think it would be
better to return VMBUS_RQST_ERROR
as a way to assert that there is a bug in the code. I don't know if
I'm missing something here.
Andres.
On Mon, Jun 29, 2020 at 07:45:00PM -0400, Andres Beltran wrote:
> On Mon, Jun 29, 2020 at 6:20 PM Wei Liu <[email protected]> wrote:
> >
> > On Mon, Jun 29, 2020 at 05:51:05PM -0400, Andres Beltran wrote:
> > > On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> > > >
> > > > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Get rid of "rqstor" variable in __vmbus_open().
> > > > >
> > > > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > > > include/linux/hyperv.h | 21 ++++++
> > > > > 2 files changed, 167 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > > > + }
> > > > > +
> > > >
> > > > Sorry for not noticing this in the last round: this infrastructure is
> > > > initialized conditionally but used unconditionally.
> > > >
> > > > I can think of two options here:
> > > >
> > > > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > > > infra.
> > > > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > > > uninitialized state.
> > > >
> > > > For #2, you can simply check rqstor->size _before_ taking the lock
> > > > (because it may be uninitialized, and the assumption is ->size will not
> > > > change during the channel's lifetime, hence no lock is needed) and
> > > > simply return the same value to the caller.
> > > >
> > > > Wei.
> > >
> > > Right. I think option #2 would be preferable in this case, because #1 works
> > > if we had a default non-zero size for cases where rqstor_size has not been
> > > set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> > > do you mean by "same value"? I think we would need to return
> > > VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> > > returning the same guest memory address which we don't want to expose.
> >
> > By "same value", I meant reverting back to using guest memory address.
> > I thought downgrading gracefully is better than making the driver stop
> > working.
> >
> > If exposing guest address is not acceptable, you can return
> > VMBUS_RQST_ERROR -- but at the point you may as well mandate requestor
> > infrastructure to be always initialized, right?
> >
>
> If the allocation of the requestor fails during runtime, vmbus_open()
> fails too and therefore,
> the channel and the requestor will not be created. So, the 2 functions
> (next_id, requestor_addr)
> will never get called, right? The only case in which we hit this edge
> case is if a driver is using this
> mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
> value before calling vmbus_open()),
Right. This is what I was getting at. Setting the size to 0 effectively
makes the driver unusable. And per your design, it should be considered
a bug.
> but that would be more like a coding bug. So, I think it would be
> better to return VMBUS_RQST_ERROR
> as a way to assert that there is a bug in the code. I don't know if
> I'm missing something here.
Since we know setting size to 0 is a bug, you can actually just do the
following in the __vmbus_open function instead of going through all the
initialization with the knowledge vmbus_next_request_id & co will fail.
/* Create and init requestor */
if (!newchannel->rqstor_size)
return an error to caller here
vmbus_alloc_requestor(...);
Wei.
>
> Andres.
On Tue, Jun 30, 2020 at 10:09:45AM +0000, Wei Liu wrote:
> On Mon, Jun 29, 2020 at 07:45:00PM -0400, Andres Beltran wrote:
> > On Mon, Jun 29, 2020 at 6:20 PM Wei Liu <[email protected]> wrote:
> > >
> > > On Mon, Jun 29, 2020 at 05:51:05PM -0400, Andres Beltran wrote:
> > > > On Mon, Jun 29, 2020 at 4:46 PM Wei Liu <[email protected]> wrote:
> > > > >
> > > > > On Mon, Jun 29, 2020 at 04:02:25PM -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]>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Get rid of "rqstor" variable in __vmbus_open().
> > > > > >
> > > > > > drivers/hv/channel.c | 146 +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/linux/hyperv.h | 21 ++++++
> > > > > > 2 files changed, 167 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > > > > index 3ebda7707e46..c89d57d0c2d2 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;
> > > > > > + }
> > > > > > +
> > > > >
> > > > > Sorry for not noticing this in the last round: this infrastructure is
> > > > > initialized conditionally but used unconditionally.
> > > > >
> > > > > I can think of two options here:
> > > > >
> > > > > 1. Mandate rqstor_size to be non-zero. Always initialize this
> > > > > infra.
> > > > > 2. Modify vmbus_next_request_id and vmbus_request_addr to deal with
> > > > > uninitialized state.
> > > > >
> > > > > For #2, you can simply check rqstor->size _before_ taking the lock
> > > > > (because it may be uninitialized, and the assumption is ->size will not
> > > > > change during the channel's lifetime, hence no lock is needed) and
> > > > > simply return the same value to the caller.
> > > > >
> > > > > Wei.
> > > >
> > > > Right. I think option #2 would be preferable in this case, because #1 works
> > > > if we had a default non-zero size for cases where rqstor_size has not been
> > > > set to a non-zero value before calling vmbus_alloc_requestor(). For #2, what
> > > > do you mean by "same value"? I think we would need to return
> > > > VMBUS_RQST_ERROR if the size is 0, because otherwise we would be
> > > > returning the same guest memory address which we don't want to expose.
> > >
> > > By "same value", I meant reverting back to using guest memory address.
> > > I thought downgrading gracefully is better than making the driver stop
> > > working.
> > >
> > > If exposing guest address is not acceptable, you can return
> > > VMBUS_RQST_ERROR -- but at the point you may as well mandate requestor
> > > infrastructure to be always initialized, right?
> > >
> >
> > If the allocation of the requestor fails during runtime, vmbus_open()
> > fails too and therefore,
> > the channel and the requestor will not be created. So, the 2 functions
> > (next_id, requestor_addr)
> > will never get called, right? The only case in which we hit this edge
> > case is if a driver is using this
> > mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
> > value before calling vmbus_open()),
>
> Right. This is what I was getting at. Setting the size to 0 effectively
> makes the driver unusable. And per your design, it should be considered
> a bug.
>
> > but that would be more like a coding bug. So, I think it would be
> > better to return VMBUS_RQST_ERROR
> > as a way to assert that there is a bug in the code. I don't know if
> > I'm missing something here.
>
> Since we know setting size to 0 is a bug, you can actually just do the
> following in the __vmbus_open function instead of going through all the
> initialization with the knowledge vmbus_next_request_id & co will fail.
>
> /* Create and init requestor */
> if (!newchannel->rqstor_size)
> return an error to caller here
>
> vmbus_alloc_requestor(...);
And obviously you should check vmbus_alloc_requestor's return value
somehow. You get the idea...
Wei.
>
>
> Wei.
>
> >
> > Andres.
On Tue, Jun 30, 2020 at 10:17:36AM +0000, Wei Liu wrote:
[...]
> > >
> > > If the allocation of the requestor fails during runtime, vmbus_open()
> > > fails too and therefore,
> > > the channel and the requestor will not be created. So, the 2 functions
> > > (next_id, requestor_addr)
> > > will never get called, right? The only case in which we hit this edge
> > > case is if a driver is using this
> > > mechanism with a size of 0 (i.e. rqstor_size is not set to a non-zero
> > > value before calling vmbus_open()),
> >
> > Right. This is what I was getting at. Setting the size to 0 effectively
> > makes the driver unusable. And per your design, it should be considered
> > a bug.
> >
> > > but that would be more like a coding bug. So, I think it would be
> > > better to return VMBUS_RQST_ERROR
> > > as a way to assert that there is a bug in the code. I don't know if
> > > I'm missing something here.
> >
> > Since we know setting size to 0 is a bug, you can actually just do the
> > following in the __vmbus_open function instead of going through all the
> > initialization with the knowledge vmbus_next_request_id & co will fail.
> >
> > /* Create and init requestor */
> > if (!newchannel->rqstor_size)
> > return an error to caller here
> >
> > vmbus_alloc_requestor(...);
>
> And obviously you should check vmbus_alloc_requestor's return value
> somehow. You get the idea...
>
Andrea pointed out that I missed one critical aspect of the design --
not all drivers are supposed to use this infrastructure. That's contrary
to my original understanding, in which all drivers are supposed to use
this infrastructure.
With that in mind, it is okay to only initialize the infra only when
->rqstor_size is not zero. Then you just handle the edge case in
vmbus_next_request_id & co.
Wei.
> Wei.
>
> >
> >
> > Wei.
> >
> > >
> > > Andres.