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

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

[Andrea Parri (Microsoft)]

Hi all,

This set is a continuation of the work for hardening the VMBus drivers
against an erroneous or malicious host.  This is based on hyperv-next.

Thanks,
  Andrea

Andrea Parri (Microsoft) (6):
  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: 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 | 45 +++++++++++++++++++++++++++------------
 drivers/hv/hyperv_vmbus.h |  2 +-
 drivers/hv/vmbus_drv.c    | 33 ++++++++++++++++------------
 include/linux/hyperv.h    |  1 +
 5 files changed, 54 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH 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>
---
 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

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

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM
> 
> __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

This change is actually zero'ing more memory than is necessary.  Only the
'msg' portion is sent to Hyper-V, so that's all that needs to be zero'ed.
But this code is not performance sensitive, and doing the tighter zero'ing
would add lines of code with no real value.  So,

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

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

[Andrea Parri]

On Sun, Dec 06, 2020 at 04:59:32PM +0000, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM
> > 
> > __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
> 
> This change is actually zero'ing more memory than is necessary.  Only the
> 'msg' portion is sent to Hyper-V, so that's all that needs to be zero'ed.
> But this code is not performance sensitive, and doing the tighter zero'ing
> would add lines of code with no real value.  So,
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Thank you for the review.

Please notice that I posted a v2 of this series:

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

Thanks,
  Andrea

[PATCH 2/6] 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

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

[Michael Kelley]

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

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

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.

> +	 */
> +
>  	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

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

[Andrea Parri]

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


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

[PATCH 3/6] 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

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

[Michael Kelley]

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

Similar comment here about providing some brief context in the commit
message on the problem that we are guarding against by removing the
double fetch.

I could see combining this patch with the previous one since the
motivation and pattern of the changes are exactly the same, just for
two different fields.

Michael

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

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

[Andrea Parri]

On Sun, Dec 06, 2020 at 05:14:18PM +0000, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM
> > 
> > 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.
> 
> Similar comment here about providing some brief context in the commit
> message on the problem that we are guarding against by removing the
> double fetch.

Will expand.


> 
> I could see combining this patch with the previous one since the
> motivation and pattern of the changes are exactly the same, just for
> two different fields.

Will consider this suggestion for v3.  Please see v2 for a related
discussion.

  Andrea

[PATCH 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_obk is non-NULL: primary_channel must be NULL if
device_obj is non-NULL, cf. vmbus_add_channel_work().

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

Re: [PATCH 4/6] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()

[Wei Liu]

On Wed, Nov 18, 2020 at 03:36:47PM +0100, Andrea Parri (Microsoft) wrote:
> 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_obk is non-NULL: primary_channel must be NULL if

device_obk -> device_obj

> device_obj is non-NULL, cf. vmbus_add_channel_work().
> 

Missing a Fixes tag?

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

Re: [PATCH 4/6] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()

[Andrea Parri]

On Tue, Nov 24, 2020 at 04:26:33PM +0000, Wei Liu wrote:
> On Wed, Nov 18, 2020 at 03:36:47PM +0100, Andrea Parri (Microsoft) wrote:
> > 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_obk is non-NULL: primary_channel must be NULL if
> 
> device_obk -> device_obj

Fixed.


> 
> > device_obj is non-NULL, cf. vmbus_add_channel_work().
> > 
> 
> Missing a Fixes tag?

Yes, I've added the tag.

Thanks,
  Andrea

RE: [PATCH 4/6] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, November 18, 2020 6:37 AM
> 
> 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_obk is non-NULL: primary_channel must be NULL if
> device_obj is non-NULL, cf. vmbus_add_channel_work().
> 
> Reported-by: Juan Vazquez <juvazq@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
>  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

Taking into the account the separate comments from Wei Liu,

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

[PATCH 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

[PATCH 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>
---
 drivers/hv/channel_mgmt.c | 30 ++++++++++++++++++------------
 drivers/hv/hyperv_vmbus.h |  2 +-
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..841c0d4e101bd 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();
 
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