On Sun, Dec 06, 2020 at 05:10:26PM +0000, Michael Kelley wrote:

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM

quoted

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.

The commit message is missing some context.  Why is a double fetch a
problem?  The comments below in the code explain why, but the why
should also be briefly explained in the commit message.

I'll integrate the commit message as suggested.

quoted

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.

The "when feasible" phrase sounds like not doing double fetches is optional in
some circumstances.  But I think we always have to protect against modification
of memory shared with the host.  So perhaps the comment should be more
precise.

I guess I was imagining situations where "avoiding the double fetch"
could just not be the most "convenient" option and where we may want
to instead opt for a "full re-validation" of the data at stake (say,
fetches separated by some "complex" call chain?).  We're certainly
in sync with the general principle of protecting the guest against
modification of memory shared with the host/hypervisor.  ;-)  I'll
amend the comment accordingly.

  Andrea