[PATCH v2 0/7] Drivers: hv: vmbus: More VMBus-hardening changes

[PATCH v2 0/7] Drivers: hv: vmbus: More VMBus-hardening changes

[Andrea Parri (Microsoft)]

Hi all,

This is v2 of [1], integrating feedback from Juan and Wei and adding
patch 4/7 (after Juan's suggestion).  Changelogs are in the patches.

Thanks,
  Andrea

[1] https://lkml.kernel.org/r/20201118143649.108465-1-parri.andrea@gmail.com

Andrea Parri (Microsoft) (7):
  Drivers: hv: vmbus: Initialize memory to be sent to the host
  Drivers: hv: vmbus: Avoid double fetch of msgtype in
    vmbus_on_msg_dpc()
  Drivers: hv: vmbus: Avoid double fetch of payload_size in
    vmbus_on_msg_dpc()
  Drivers: hv: vmbus: Copy the hv_message object in vmbus_on_msg_dpc()
  Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()
  Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
  Drivers: hv: vmbus: Do not allow overwriting
    vmbus_connection.channels[]

 drivers/hv/channel.c      |  4 +--
 drivers/hv/channel_mgmt.c | 53 +++++++++++++++++++++++++++------------
 drivers/hv/hyperv_vmbus.h |  2 +-
 drivers/hv/vmbus_drv.c    | 43 ++++++++++++++++++-------------
 include/linux/hyperv.h    |  1 +
 5 files changed, 67 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v2 1/7] Drivers: hv: vmbus: Initialize memory to be sent to the host

[Andrea Parri (Microsoft)]

__vmbus_open() and vmbus_teardown_gpadl() do not inizialite the memory
for the vmbus_channel_open_channel and the vmbus_channel_gpadl_teardown
objects they allocate respectively.  These objects contain padding bytes
and fields that are left uninitialized and that are later sent to the
host, potentially leaking guest data.  Zero initialize such fields to
avoid leaking sensitive information to the host.

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/channel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0d63862d65518..9aa789e5f22bb 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -621,7 +621,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 		goto error_clean_ring;
 
 	/* Create and init the channel open message */
-	open_info = kmalloc(sizeof(*open_info) +
+	open_info = kzalloc(sizeof(*open_info) +
 			   sizeof(struct vmbus_channel_open_channel),
 			   GFP_KERNEL);
 	if (!open_info) {
@@ -748,7 +748,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
 	unsigned long flags;
 	int ret;
 
-	info = kmalloc(sizeof(*info) +
+	info = kzalloc(sizeof(*info) +
 		       sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
-- 
2.25.1

[PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()

[Andrea Parri (Microsoft)]

vmbus_on_msg_dpc() double fetches from msgtype.  The double fetch can
lead to an out-of-bound access when accessing the channel_message_table
array.  In turn, the use of the out-of-bound entry could lead to code
execution primitive (entry->message_handler()).  Avoid the double fetch
by saving the value of msgtype into a local variable.

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/vmbus_drv.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0a2711aa63a15..82b23baa446d7 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1057,6 +1057,7 @@ void vmbus_on_msg_dpc(unsigned long data)
 	struct hv_message *msg = (struct hv_message *)page_addr +
 				  VMBUS_MESSAGE_SINT;
 	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;
@@ -1072,12 +1073,19 @@ 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;
+	msgtype = hdr->msgtype;
 
 	trace_vmbus_on_msg_dpc(hdr);
 
-	if (hdr->msgtype >= CHANNELMSG_COUNT) {
-		WARN_ONCE(1, "unknown msgtype=%d\n", hdr->msgtype);
+	if (msgtype >= CHANNELMSG_COUNT) {
+		WARN_ONCE(1, "unknown msgtype=%d\n", msgtype);
 		goto msg_handled;
 	}
 
@@ -1087,14 +1095,14 @@ void vmbus_on_msg_dpc(unsigned long data)
 		goto msg_handled;
 	}
 
-	entry = &channel_message_table[hdr->msgtype];
+	entry = &channel_message_table[msgtype];
 
 	if (!entry->message_handler)
 		goto msg_handled;
 
 	if (msg->header.payload_size < entry->min_payload_len) {
 		WARN_ONCE(1, "message too short: msgtype=%d len=%d\n",
-			  hdr->msgtype, msg->header.payload_size);
+			  msgtype, msg->header.payload_size);
 		goto msg_handled;
 	}
 
@@ -1115,7 +1123,7 @@ void vmbus_on_msg_dpc(unsigned long data)
 		 * by offer_in_progress and by channel_mutex.  See also the
 		 * inline comments in vmbus_onoffer_rescind().
 		 */
-		switch (hdr->msgtype) {
+		switch (msgtype) {
 		case CHANNELMSG_RESCIND_CHANNELOFFER:
 			/*
 			 * If we are handling the rescind message;
-- 
2.25.1

[PATCH v2 3/7] Drivers: hv: vmbus: Avoid double fetch of payload_size in vmbus_on_msg_dpc()

[Andrea Parri (Microsoft)]

vmbus_on_msg_dpc() double fetches from payload_size.  The double fetch
can lead to a buffer overflow when (mem)copying the hv_message object.
Avoid the double fetch by saving the value of payload_size into a local
variable.

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/vmbus_drv.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 82b23baa446d7..0e39f1d6182e9 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1056,6 +1056,7 @@ void vmbus_on_msg_dpc(unsigned long data)
 	void *page_addr = hv_cpu->synic_message_page;
 	struct hv_message *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;
@@ -1089,9 +1090,8 @@ void vmbus_on_msg_dpc(unsigned long data)
 		goto msg_handled;
 	}
 
-	if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
-		WARN_ONCE(1, "payload size is too large (%d)\n",
-			  msg->header.payload_size);
+	if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
+		WARN_ONCE(1, "payload size is too large (%d)\n", payload_size);
 		goto msg_handled;
 	}
 
@@ -1100,21 +1100,18 @@ void vmbus_on_msg_dpc(unsigned long data)
 	if (!entry->message_handler)
 		goto msg_handled;
 
-	if (msg->header.payload_size < entry->min_payload_len) {
-		WARN_ONCE(1, "message too short: msgtype=%d len=%d\n",
-			  msgtype, msg->header.payload_size);
+	if (payload_size < entry->min_payload_len) {
+		WARN_ONCE(1, "message too short: msgtype=%d len=%d\n", msgtype, payload_size);
 		goto msg_handled;
 	}
 
 	if (entry->handler_type	== VMHT_BLOCKING) {
-		ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
-			      GFP_ATOMIC);
+		ctx = kmalloc(sizeof(*ctx) + payload_size, GFP_ATOMIC);
 		if (ctx == NULL)
 			return;
 
 		INIT_WORK(&ctx->work, vmbus_onmessage_work);
-		memcpy(&ctx->msg, msg, sizeof(msg->header) +
-		       msg->header.payload_size);
+		memcpy(&ctx->msg, msg, sizeof(msg->header) + payload_size);
 
 		/*
 		 * The host can generate a rescind message while we
-- 
2.25.1

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

[Andrea Parri (Microsoft)]

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.

Suggested-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 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

[PATCH v2 5/7] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()

[Andrea Parri (Microsoft)]

When channel->device_obj is non-NULL, vmbus_onoffer_rescind() could
invoke put_device(), that will eventually release the device and free
the channel object (cf. vmbus_device_release()).  However, a pointer
to the object is dereferenced again later to load the primary_channel.
The use-after-free can be avoided by noticing that this load/check is
redundant if device_obj is non-NULL: primary_channel must be NULL if
device_obj is non-NULL, cf. vmbus_add_channel_work().

Fixes: 54a66265d6754b ("Drivers: hv: vmbus: Fix rescind handling")
Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Changes since v1:
  - Fix typo in commit message
  - Add Fixes: tag

 drivers/hv/channel_mgmt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 5bc5eef5da159..4072fd1f22146 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1116,8 +1116,7 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
 			vmbus_device_unregister(channel->device_obj);
 			put_device(dev);
 		}
-	}
-	if (channel->primary_channel != NULL) {
+	} else if (channel->primary_channel != NULL) {
 		/*
 		 * Sub-channel is being rescinded. Following is the channel
 		 * close sequence when initiated from the driveri (refer to
-- 
2.25.1

[PATCH v2 6/7] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()

[Andrea Parri (Microsoft)]

An erroneous or malicious host could send multiple rescind messages for
a same channel.  In vmbus_onoffer_rescind(), the guest maps the channel
ID to obtain a pointer to the channel object and it eventually releases
such object and associated data.  The host could time rescind messages
and lead to an use-after-free.  Add a new flag to the channel structure
to make sure that only one instance of vmbus_onoffer_rescind() can get
the reference to the channel object.

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
 drivers/hv/channel_mgmt.c | 12 ++++++++++++
 include/linux/hyperv.h    |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 4072fd1f22146..68950a1e4b638 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1063,6 +1063,18 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
 
 	mutex_lock(&vmbus_connection.channel_mutex);
 	channel = relid2channel(rescind->child_relid);
+	if (channel != NULL) {
+		/*
+		 * Guarantee that no other instance of vmbus_onoffer_rescind()
+		 * has got a reference to the channel object.  Synchronize on
+		 * &vmbus_connection.channel_mutex.
+		 */
+		if (channel->rescind_ref) {
+			mutex_unlock(&vmbus_connection.channel_mutex);
+			return;
+		}
+		channel->rescind_ref = true;
+	}
 	mutex_unlock(&vmbus_connection.channel_mutex);
 
 	if (channel == NULL) {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 2ea967bc17adf..f0d48a368f131 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -809,6 +809,7 @@ struct vmbus_channel {
 	u8 monitor_bit;
 
 	bool rescind; /* got rescind msg */
+	bool rescind_ref; /* got rescind msg, got channel reference */
 	struct completion rescind_event;
 
 	u32 ringbuffer_gpadlhandle;
-- 
2.25.1

[PATCH v2 7/7] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[]

[Andrea Parri (Microsoft)]

Currently, vmbus_onoffer() and vmbus_process_offer() are not validating
whether a given entry in the vmbus_connection.channels[] array is empty
before filling the entry with a call of vmbus_channel_map_relid().  An
erroneous or malicious host could rely on this to leak channel objects.
Do not allow overwriting an entry vmbus_connection.channels[].

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Changes since v1:
  - Don't corrupt oldchannel if offer->child_relid is invalid

 drivers/hv/channel_mgmt.c | 38 ++++++++++++++++++++++++--------------
 drivers/hv/hyperv_vmbus.h |  2 +-
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..f91d476dfe381 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -354,10 +354,12 @@ static void free_channel(struct vmbus_channel *channel)
 	kobject_put(&channel->kobj);
 }
 
-void vmbus_channel_map_relid(struct vmbus_channel *channel)
+int vmbus_channel_map_relid(struct vmbus_channel *channel)
 {
-	if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
-		return;
+	u32 relid = channel->offermsg.child_relid;
+
+	if (WARN_ON(relid >= MAX_CHANNEL_RELIDS || vmbus_connection.channels[relid] != NULL))
+		return -EINVAL;
 	/*
 	 * The mapping of the channel's relid is visible from the CPUs that
 	 * execute vmbus_chan_sched() by the time that vmbus_chan_sched() will
@@ -383,18 +385,17 @@ void vmbus_channel_map_relid(struct vmbus_channel *channel)
 	 *      of the VMBus driver and vmbus_chan_sched() can not run before
 	 *      vmbus_bus_resume() has completed execution (cf. resume_noirq).
 	 */
-	smp_store_mb(
-		vmbus_connection.channels[channel->offermsg.child_relid],
-		channel);
+	smp_store_mb(vmbus_connection.channels[relid], channel);
+	return 0;
 }
 
 void vmbus_channel_unmap_relid(struct vmbus_channel *channel)
 {
-	if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
+	u32 relid = channel->offermsg.child_relid;
+
+	if (WARN_ON(relid >= MAX_CHANNEL_RELIDS))
 		return;
-	WRITE_ONCE(
-		vmbus_connection.channels[channel->offermsg.child_relid],
-		NULL);
+	WRITE_ONCE(vmbus_connection.channels[relid], NULL);
 }
 
 static void vmbus_release_relid(u32 relid)
@@ -601,6 +602,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 	 */
 	atomic_dec(&vmbus_connection.offer_in_progress);
 
+	if (vmbus_channel_map_relid(newchannel)) {
+		mutex_unlock(&vmbus_connection.channel_mutex);
+		kfree(newchannel);
+		return;
+	}
+
 	list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
 		if (guid_equal(&channel->offermsg.offer.if_type,
 			       &newchannel->offermsg.offer.if_type) &&
@@ -619,6 +626,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 		 * Check to see if this is a valid sub-channel.
 		 */
 		if (newchannel->offermsg.offer.sub_channel_index == 0) {
+			vmbus_channel_unmap_relid(newchannel);
 			mutex_unlock(&vmbus_connection.channel_mutex);
 			/*
 			 * Don't call free_channel(), because newchannel->kobj
@@ -635,8 +643,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
 		list_add_tail(&newchannel->sc_list, &channel->sc_list);
 	}
 
-	vmbus_channel_map_relid(newchannel);
-
 	mutex_unlock(&vmbus_connection.channel_mutex);
 	cpus_read_unlock();
 
@@ -920,6 +926,8 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
 	oldchannel = find_primary_channel_by_offer(offer);
 
 	if (oldchannel != NULL) {
+		u32 relid = offer->child_relid;
+
 		/*
 		 * We're resuming from hibernation: all the sub-channel and
 		 * hv_sock channels we had before the hibernation should have
@@ -954,8 +962,10 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
 		atomic_dec(&vmbus_connection.offer_in_progress);
 
 		WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
+		if (WARN_ON(vmbus_connection.channels[relid] != NULL))
+			return;
 		/* Fix up the relid. */
-		oldchannel->offermsg.child_relid = offer->child_relid;
+		oldchannel->offermsg.child_relid = relid;
 
 		offer_sz = sizeof(*offer);
 		if (memcmp(offer, &oldchannel->offermsg, offer_sz) != 0) {
@@ -967,7 +977,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
 			 * reoffers the device upon resume.
 			 */
 			pr_debug("vmbus offer changed: relid=%d\n",
-				 offer->child_relid);
+				 relid);
 
 			print_hex_dump_debug("Old vmbus offer: ",
 					     DUMP_PREFIX_OFFSET, 16, 4,
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index e2064bf2b557d..89d7b95b3bdad 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -338,7 +338,7 @@ int vmbus_add_channel_kobj(struct hv_device *device_obj,
 
 void vmbus_remove_channel_attr_group(struct vmbus_channel *channel);
 
-void vmbus_channel_map_relid(struct vmbus_channel *channel);
+int vmbus_channel_map_relid(struct vmbus_channel *channel);
 void vmbus_channel_unmap_relid(struct vmbus_channel *channel);
 
 struct vmbus_channel *relid2channel(u32 relid);
-- 
2.25.1

Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()

[Wei Liu]

On Wed, Dec 02, 2020 at 10:22:09AM +0100, Andrea Parri (Microsoft) wrote:
> vmbus_on_msg_dpc() double fetches from msgtype.  The double fetch can
> lead to an out-of-bound access when accessing the channel_message_table
> array.  In turn, the use of the out-of-bound entry could lead to code
> execution primitive (entry->message_handler()).  Avoid the double fetch
> by saving the value of msgtype into a local variable.
> 
> Reported-by: Juan Vazquez <juvazq@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  drivers/hv/vmbus_drv.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 0a2711aa63a15..82b23baa446d7 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1057,6 +1057,7 @@ void vmbus_on_msg_dpc(unsigned long data)
>  	struct hv_message *msg = (struct hv_message *)page_addr +
>  				  VMBUS_MESSAGE_SINT;
>  	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;
> @@ -1072,12 +1073,19 @@ 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;
> +	msgtype = hdr->msgtype;

Should READ_ONCE be used here?

Wei.

Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()

[Andrea Parri]

> > @@ -1072,12 +1073,19 @@ 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;
> > +	msgtype = hdr->msgtype;
> 
> Should READ_ONCE be used here?

I think it should.  Thank you for pointing this out.

  Andrea

Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()

[Wei Liu]

On Wed, Dec 02, 2020 at 02:37:16PM +0100, Andrea Parri wrote:
> > > @@ -1072,12 +1073,19 @@ 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;
> > > +	msgtype = hdr->msgtype;
> > 
> > Should READ_ONCE be used here?
> 
> I think it should.  Thank you for pointing this out.

Glad I can help.

The same comment applies to other patches as well, of course.

Wei.

> 
>   Andrea

Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()

[Andrea Parri]

On Wed, Dec 02, 2020 at 01:40:04PM +0000, Wei Liu wrote:
> On Wed, Dec 02, 2020 at 02:37:16PM +0100, Andrea Parri wrote:
> > > > @@ -1072,12 +1073,19 @@ 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;
> > > > +	msgtype = hdr->msgtype;
> > > 
> > > Should READ_ONCE be used here?
> > 
> > I think it should.  Thank you for pointing this out.
> 
> Glad I can help.
> 
> The same comment applies to other patches as well, of course.

(As discussed offline/for reference:) I can spot a similar case in
patch #3; however, #4 is supposed to make that access 'non-shared'.

I should probably just squash patches #3 and #4; I'll try to do so
in v3...

Thanks,
  Andrea

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@gmail.com> 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@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  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

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

[Andrea Parri]

On Sun, Dec 06, 2020 at 06:39:39PM +0000, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> 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.

[...]

> 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.

I'm okay with this: I will revisit the commit messages of 2/3, maybe
squash those 'simplifications' into a single patch, and then 'solve'
/guard against the double fetch problem with 4.

Thanks,
  Andrea