RE: [PATCH v2 4/7] Drivers: hv: vmbus: Copy the hv_message object in vmbus_on_msg_dpc()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx> Sent: Wednesday, December 2, 2020 1:22 AM
> 
> The hv_message object is in memory shared with the host.  To prevent
> an erroneous or a malicious host from 'corrupting' such object, copy
> the object into private memory.

Sorry for reviewing the older version of this patch series. :-(

To me, adding this patch to the series introduces some motivational
inconsistencies.  The top level problem is that Hyper-V could change the
message while it is sitting in memory shared between the guest and host.
Patches 2 and 3 of this series guard against that problem, at least for 
the msgtype in the payload (Patch 2) and the payload_size in the header
(Patch 3).  While a copy of the message is made for the VMHT_BLOCKING
case, in the other cases where the message_handler is invoked directly,
the message handler may have its own double-fetch problems.  Rather than
try to fix all double-fetch problems scattered throughout the message_handlers,
it's easier to make a local copy of the entire messages here at the start
of vmbus_on_msg_dpc.  This code is not performance sensitive, so
making the copy is a lot safer in the long run.  Hence I'm good with adding
this Patch 4 as the best solution.  Perhaps the commit message should
reflect that the copy is being made not just to ensure vmbus_on_msg_dpc()
is correct, but also to ensure that individual message_handlers don't have
to deal with the problem.

But if we're going to just make a copy at the start and use the copy for
everything, then the motivation for the changes in Patches 2 and 3 goes
away.  The double-fetch problem is solved entirely by Patch 4.  The
changes in Patches 2 and 3 *are* nice for simplifying the code, but
that's all.  The code simplification is still useful as prep to reduce the
number of references to "msg" that have to be changed to "msg_copy",
but the commit message should be changed to reflect that, rather than
to eliminate double fetches.

Having to make a second copy for the VMHT_BLOCKING case is a bit
distasteful, but I don't have a clever solution, and again, this code is
not performance sensitive.

Finally, I'll note that the call to vmbus_signal_eom() at the end of
vmbus_on_msg_dpc() cannot use the copy.  It has to update the
original message that is shared with Hyper-V.  Your patch is already
correct on that point.

Michael

> 
> Suggested-by: Juan Vazquez <juvazq@xxxxxxxxxxxxx>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx>
> ---
>  drivers/hv/vmbus_drv.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 0e39f1d6182e9..796202aa7f9b4 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1054,14 +1054,24 @@ void vmbus_on_msg_dpc(unsigned long data)
>  {
>  	struct hv_per_cpu_context *hv_cpu = (void *)data;
>  	void *page_addr = hv_cpu->synic_message_page;
> -	struct hv_message *msg = (struct hv_message *)page_addr +
> +	struct hv_message msg_copy, *msg = (struct hv_message *)page_addr +
>  				  VMBUS_MESSAGE_SINT;
> -	__u8 payload_size = msg->header.payload_size;
>  	struct vmbus_channel_message_header *hdr;
>  	enum vmbus_channel_message_type msgtype;
>  	const struct vmbus_channel_message_table_entry *entry;
>  	struct onmessage_work_context *ctx;
> -	u32 message_type = msg->header.message_type;
> +	__u8 payload_size;
> +	u32 message_type;
> +
> +	/*
> +	 * The hv_message object is in memory shared with the host.  Prevent an
> +	 * erroneous or malicious host from 'corrupting' such object by copying
> +	 * the object to private memory.
> +	 */
> +	memcpy(&msg_copy, msg, sizeof(struct hv_message));
> +
> +	payload_size = msg_copy.header.payload_size;
> +	message_type = msg_copy.header.message_type;
> 
>  	/*
>  	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
> @@ -1074,13 +1084,7 @@ void vmbus_on_msg_dpc(unsigned long data)
>  		/* no msg */
>  		return;
> 
> -	/*
> -	 * The hv_message object is in memory shared with the host.  The host
> -	 * could erroneously or maliciously modify such object.  Make sure to
> -	 * validate its fields and avoid double fetches whenever feasible.
> -	 */
> -
> -	hdr = (struct vmbus_channel_message_header *)msg->u.payload;
> +	hdr = (struct vmbus_channel_message_header *)msg_copy.u.payload;
>  	msgtype = hdr->msgtype;
> 
>  	trace_vmbus_on_msg_dpc(hdr);
> @@ -1111,7 +1115,7 @@ void vmbus_on_msg_dpc(unsigned long data)
>  			return;
> 
>  		INIT_WORK(&ctx->work, vmbus_onmessage_work);
> -		memcpy(&ctx->msg, msg, sizeof(msg->header) + payload_size);
> +		memcpy(&ctx->msg, &msg_copy, sizeof(msg_copy.header) +
> payload_size);
> 
>  		/*
>  		 * The host can generate a rescind message while we
> --
> 2.25.1