From: Andres Beltran <lkmlabelt@xxxxxxxxx> Sent: Wednesday, July 22, 2020 11:11 AM
>
> 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 <lkmlabelt@xxxxxxxxx>
> ---
> Changes in v5:
> - Add support for unsolicited messages sent by the host with a
> request ID of 0.
> Changes in v4:
> - Use channel->rqstor_size to check if rqstor has been
> initialized.
> Changes in v3:
> - Check that requestor has been initialized in
> vmbus_next_request_id() and vmbus_request_addr().
> Changes in v2:
> - Get rid of "rqstor" variable in __vmbus_open().
>
> drivers/hv/channel.c | 175 +++++++++++++++++++++++++++++++++++++++++
> include/linux/hyperv.h | 21 +++++
> 2 files changed, 196 insertions(+)
These changes do indeed solve the previously reported problem, which
is good. I've tested on my own WSLv2 installation, and everything works.
But see comments further down.
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 3ebda7707e46..b0a607ec4a37 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -112,6 +112,71 @@ 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 + 2;
I don't think the above does what you want. The allocated
array ends up as follows:
Slot 0 contains "2"
Slot 1 contains "3"
...
Slot size-2 contains size
Slot size-1 contains U64_MAX
This means that allocating the next-to-last entry will go
awry. I think the previous version of the slot initialization
code will actually work just fine.
> +
> + /* Last slot (no more available slots) */
> + req_arr[i] = U64_MAX;
> +
> + return req_arr;
> +}
> +
> +/*
> + * vmbus_alloc_requestor - Initializes @rqstor's fields.
> + * Start the ID count at 1 because Hyper-V can send an unsolicited message
> + * with request ID of 0.
> + * @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 = 1;
> + 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 +197,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 +299,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 +775,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 +1012,103 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel,
> void *buffer,
> buffer_actual_len, requestid, true);
> }
> EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> +
> +/*
> + * vmbus_requestor_hash_idx - Returns the index in the requestor array
> + * that rqst_id maps to.
> + */
> +static inline u64 vmbus_requestor_hash_idx(const u64 rqst_id)
> +{
> + return rqst_id - 1;
> +}
The overall scheme you are using to handle the 0 transactionID is
a good one. Basically the slot array is still tracking values 0 thru
size-1, but what is presented to the calling VMbus driver is values
in the range 1 thru size. That way you can recognize 0 as a special case.
So take this implementation approach:
* Start with the previous version of the vmbus_next_request_id()
and vmbus_request_addr() code.
* In vmbus_next_request_id(), just return current_id+1 instead of
current_id.
* In vmbus_request_addr(), add the new code that checks trans_id
for 0 and returns immediately. Otherwise, decrement trans_id by 1
and proceed with the existing code.
With this approach, none of the initialization code needs to change.
Everything uses values in the range 0 to size-1, except that what is
presented to the VMbus drivers is shifted higher by 1.
Hopefully, I'm thinking about this correctly.
Michael
> +
> +/*
> + * 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, idx;
> + const struct vmbus_channel *channel =
> + container_of(rqstor, const struct vmbus_channel, requestor);
> +
> + /* Check rqstor has been initialized */
> + if (!channel->rqstor_size)
> + return VMBUS_RQST_ERROR;
> +
> + spin_lock_irqsave(&rqstor->req_lock, flags);
> + current_id = rqstor->next_request_id;
> + idx = vmbus_requestor_hash_idx(current_id);
> +
> + /* Requestor array is full */
> + if (current_id > rqstor->size) {
> + current_id = VMBUS_RQST_ERROR;
> + goto exit;
> + }
> +
> + rqstor->next_request_id = rqstor->req_arr[idx];
> + rqstor->req_arr[idx] = rqst_addr;
> +
> + /* The already held spin lock provides atomicity */
> + bitmap_set(rqstor->req_bitmap, idx, 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, idx;
> + const struct vmbus_channel *channel =
> + container_of(rqstor, const struct vmbus_channel, requestor);
> +
> + /* Check rqstor has been initialized */
> + if (!channel->rqstor_size)
> + return VMBUS_RQST_ERROR;
> +
> + /* Hyper-V can send an unsolicited message with tid of 0 */
> + if (!trans_id)
> + return trans_id;
> +
> + spin_lock_irqsave(&rqstor->req_lock, flags);
> +
> + /* Invalid trans_id */
> + if (trans_id > rqstor->size) {
> + req_addr = VMBUS_RQST_ERROR;
> + goto exit;
> + }
> +
> + idx = vmbus_requestor_hash_idx(trans_id);
> +
> + /* Invalid trans_id: empty slot */
> + if (!test_bit(idx, rqstor->req_bitmap)) {
> + req_addr = VMBUS_RQST_ERROR;
> + goto exit;
> + }
> +
> + req_addr = rqstor->req_arr[idx];
> + rqstor->req_arr[idx] = rqstor->next_request_id;
> + rqstor->next_request_id = trans_id;
> +
> + /* The already held spin lock provides atomicity */
> + bitmap_clear(rqstor->req_bitmap, idx, 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
