[PATCH v3 0/6] Drivers: hv: vmbus: More VMBus-hardening changes

[PATCH v3 0/6] Drivers: hv: vmbus: More VMBus-hardening changes

[Andrea Parri (Microsoft)]

Integrating feedback from Juan, Michael and Wei. [1]  Changelogs are
inline/in the patches.

Thanks,
  Andrea

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

Andrea Parri (Microsoft) (6):
  Drivers: hv: vmbus: Initialize memory to be sent to the host
  Drivers: hv: vmbus: Reduce number of references to message in
    vmbus_on_msg_dpc()
  Drivers: hv: vmbus: Copy the hv_message 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 | 55 +++++++++++++++++++++++++++------------
 drivers/hv/hyperv_vmbus.h |  2 +-
 drivers/hv/vmbus_drv.c    | 43 ++++++++++++++++++------------
 include/linux/hyperv.h    |  1 +
 5 files changed, 69 insertions(+), 36 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] 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>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
Changes since v2:
  - Add Reviewed-by: tag

 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 v3 2/6] Drivers: hv: vmbus: Reduce number of references to message in vmbus_on_msg_dpc()

[Andrea Parri (Microsoft)]

Simplify the function by removing various references to the hv_message
'msg', introduce local variables 'msgtype' and 'payload_size'.

Suggested-by: Juan Vazquez <juvazq@microsoft.com>
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Changes since v2:
  - Squash patches #2 and #3
  - Revisit commit message

 drivers/hv/vmbus_drv.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 502f8cd95f6d4..44bcf9ccdaf5f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1057,9 +1057,11 @@ 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;
+	__u8 payload_size;
 
 	/*
 	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
@@ -1073,40 +1075,38 @@ void vmbus_on_msg_dpc(unsigned long data)
 		return;
 
 	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;
 	}
 
-	if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
-		WARN_ONCE(1, "payload size is too large (%d)\n",
-			  msg->header.payload_size);
+	payload_size = 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;
 	}
 
-	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);
+	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
@@ -1115,7 +1115,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

RE: [PATCH v3 2/6] Drivers: hv: vmbus: Reduce number of references to message in vmbus_on_msg_dpc()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Tuesday, December 8, 2020 11:08 PM
> 
> Simplify the function by removing various references to the hv_message
> 'msg', introduce local variables 'msgtype' and 'payload_size'.
> 
> Suggested-by: Juan Vazquez <juvazq@microsoft.com>
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> Changes since v2:
>   - Squash patches #2 and #3
>   - Revisit commit message
> 
>  drivers/hv/vmbus_drv.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 502f8cd95f6d4..44bcf9ccdaf5f 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1057,9 +1057,11 @@ 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;
> +	__u8 payload_size;
> 
>  	/*
>  	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
> @@ -1073,40 +1075,38 @@ void vmbus_on_msg_dpc(unsigned long data)
>  		return;
> 
>  	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;
>  	}
> 
> -	if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
> -		WARN_ONCE(1, "payload size is too large (%d)\n",
> -			  msg->header.payload_size);
> +	payload_size = 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;
>  	}
> 
> -	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);
> +	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
> @@ -1115,7 +1115,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

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH v3 3/6] Drivers: hv: vmbus: Copy the hv_message in vmbus_on_msg_dpc()

[Andrea Parri (Microsoft)]

Since the message is in memory shared with the host, an erroneous or a
malicious Hyper-V could 'corrupt' the message while vmbus_on_msg_dpc()
or individual message handlers are executing.  To prevent it, copy the
message into private memory.

Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Changes since v2:
  - Revisit commit message and inline comment

 drivers/hv/vmbus_drv.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 44bcf9ccdaf5f..b1c5a89d75f9d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1054,14 +1054,14 @@ 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;
 	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;
 
 	/*
 	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
@@ -1070,11 +1070,20 @@ void vmbus_on_msg_dpc(unsigned long data)
 	 */
 	BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
 
+	/*
+	 * Since the message is in memory shared with the host, an erroneous or
+	 * malicious Hyper-V could modify the message while vmbus_on_msg_dpc()
+	 * or individual message handlers are executing; to prevent this, copy
+	 * the message into private memory.
+	 */
+	memcpy(&msg_copy, msg, sizeof(struct hv_message));
+
+	message_type = msg_copy.header.message_type;
 	if (message_type == HVMSG_NONE)
 		/* no msg */
 		return;
 
-	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);
@@ -1084,7 +1093,7 @@ void vmbus_on_msg_dpc(unsigned long data)
 		goto msg_handled;
 	}
 
-	payload_size = msg->header.payload_size;
+	payload_size = msg_copy.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;
@@ -1106,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->header) + payload_size);
 
 		/*
 		 * The host can generate a rescind message while we
-- 
2.25.1

RE: [PATCH v3 3/6] Drivers: hv: vmbus: Copy the hv_message in vmbus_on_msg_dpc()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Tuesday, December 8, 2020 11:08 PM
> 
> Since the message is in memory shared with the host, an erroneous or a
> malicious Hyper-V could 'corrupt' the message while vmbus_on_msg_dpc()
> or individual message handlers are executing.  To prevent it, copy the
> message into private memory.
> 
> Reported-by: Juan Vazquez <juvazq@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> Changes since v2:
>   - Revisit commit message and inline comment
> 
>  drivers/hv/vmbus_drv.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 44bcf9ccdaf5f..b1c5a89d75f9d 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1054,14 +1054,14 @@ 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;
>  	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;
> 
>  	/*
>  	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
> @@ -1070,11 +1070,20 @@ void vmbus_on_msg_dpc(unsigned long data)
>  	 */
>  	BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
> 
> +	/*
> +	 * Since the message is in memory shared with the host, an erroneous or
> +	 * malicious Hyper-V could modify the message while vmbus_on_msg_dpc()
> +	 * or individual message handlers are executing; to prevent this, copy
> +	 * the message into private memory.
> +	 */
> +	memcpy(&msg_copy, msg, sizeof(struct hv_message));
> +
> +	message_type = msg_copy.header.message_type;
>  	if (message_type == HVMSG_NONE)
>  		/* no msg */
>  		return;
> 
> -	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);
> @@ -1084,7 +1093,7 @@ void vmbus_on_msg_dpc(unsigned long data)
>  		goto msg_handled;
>  	}
> 
> -	payload_size = msg->header.payload_size;
> +	payload_size = msg_copy.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;
> @@ -1106,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->header) + payload_size);
> 
>  		/*
>  		 * The host can generate a rescind message while we
> --
> 2.25.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH v3 4/6] 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>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
Changes since v2:
  - Add Reviewed-by: 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 v3 5/6] 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

RE: [PATCH v3 5/6] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Tuesday, December 8, 2020 11:08 PM
> 
> 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

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH v3 6/6] 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 v2:
  - Release channel_mutex before 'return' in vmbus_onoffer() error path

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

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..2c15693b86f1e 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,12 @@ 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)) {
+			mutex_unlock(&vmbus_connection.channel_mutex);
+			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 +979,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 42f3d9d123a12..3222fbf2a21c6 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -337,7 +337,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 v3 0/6] Drivers: hv: vmbus: More VMBus-hardening changes

[Wei Liu]

On Wed, Dec 09, 2020 at 08:08:21AM +0100, Andrea Parri (Microsoft) wrote:
> Integrating feedback from Juan, Michael and Wei. [1]  Changelogs are
> inline/in the patches.
> 
> Thanks,
>   Andrea
> 
> [1] https://lkml.kernel.org/r/20201202092214.13520-1-parri.andrea@gmail.com
> 
> Andrea Parri (Microsoft) (6):
>   Drivers: hv: vmbus: Initialize memory to be sent to the host
>   Drivers: hv: vmbus: Reduce number of references to message in
>     vmbus_on_msg_dpc()
>   Drivers: hv: vmbus: Copy the hv_message 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()

I've applied the first five to hyperv-next.

>   Drivers: hv: vmbus: Do not allow overwriting
>     vmbus_connection.channels[]

This one lacks a review-by tag.

Wei.

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