RE: [PATCH v5 1/3] Drivers: hv: vmbus: Add vmbus_requestor data structure for VMBus hardening

[Michael Kelley <mikelley@microsoft.com>]

From: Andres Beltran <lkmlabelt@gmail.com> 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@gmail.com>
> ---
> 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