All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
To: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Cc: "K . Y . Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>,
	Michael Kelley <mikelley@microsoft.com>,
	Juan Vazquez <juvazq@microsoft.com>,
	Saruhan Karademir <skarade@microsoft.com>,
	"Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
Subject: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()
Date: Wed,  2 Dec 2020 10:22:09 +0100	[thread overview]
Message-ID: <20201202092214.13520-3-parri.andrea@gmail.com> (raw)
In-Reply-To: <20201202092214.13520-1-parri.andrea@gmail.com>

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


  parent reply	other threads:[~2020-12-02  9:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-02  9:22 [PATCH v2 0/7] Drivers: hv: vmbus: More VMBus-hardening changes Andrea Parri (Microsoft)
2020-12-02  9:22 ` [PATCH v2 1/7] Drivers: hv: vmbus: Initialize memory to be sent to the host Andrea Parri (Microsoft)
2020-12-02  9:22 ` Andrea Parri (Microsoft) [this message]
2020-12-02 12:22   ` [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc() Wei Liu
2020-12-02 13:37     ` Andrea Parri
2020-12-02 13:40       ` Wei Liu
2020-12-02 14:14         ` Andrea Parri
2020-12-02  9:22 ` [PATCH v2 3/7] Drivers: hv: vmbus: Avoid double fetch of payload_size " Andrea Parri (Microsoft)
2020-12-02  9:22 ` [PATCH v2 4/7] Drivers: hv: vmbus: Copy the hv_message object " Andrea Parri (Microsoft)
2020-12-06 18:39   ` Michael Kelley
2020-12-06 21:33     ` Andrea Parri
2020-12-02  9:22 ` [PATCH v2 5/7] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind() Andrea Parri (Microsoft)
2020-12-02  9:22 ` [PATCH v2 6/7] Drivers: hv: vmbus: Resolve race condition " Andrea Parri (Microsoft)
2020-12-02  9:22 ` [PATCH v2 7/7] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[] Andrea Parri (Microsoft)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201202092214.13520-3-parri.andrea@gmail.com \
    --to=parri.andrea@gmail.com \
    --cc=haiyangz@microsoft.com \
    --cc=juvazq@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=skarade@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=wei.liu@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.