[PATCH 0/5] Drivers: hv: cleanup VMBus messages handling

[PATCH 0/5] Drivers: hv: cleanup VMBus messages handling

[Vitaly Kuznetsov]

A small cleanup series mostly aimed at sanitizing memory we pass to
message handlers: not passing garbage/lefrtovers from other messages
and making sure we fail early when hypervisor misbehaves.

No (real) functional change intended.

Vitaly Kuznetsov (5):
  Drivers: hv: copy from message page only what's needed
  Drivers: hv: allocate the exact needed memory for messages
  Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
  Drivers: hv: make sure that 'struct vmbus_channel_message_header'
    compiles correctly
  Drivers: hv: check VMBus messages lengths

 drivers/hv/channel_mgmt.c | 61 ++++++++++++++++++++-------------------
 drivers/hv/hyperv_vmbus.h |  1 +
 drivers/hv/vmbus_drv.c    | 34 +++++++++++++++++-----
 include/linux/hyperv.h    |  2 +-
 4 files changed, 60 insertions(+), 38 deletions(-)

-- 
2.25.1

[PATCH 1/5] Drivers: hv: copy from message page only what's needed

[Vitaly Kuznetsov]

Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
messages. Each message comes with a header (16 bytes) which specifies the
payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
look at the real message length and copies the whole slot to a temporary
buffer before passing it to message handlers. This is potentially dangerous
as hypervisor doesn't have to clean the whole slot when putting a new
message there and a message handler can get access to some data which
belongs to a previous message.

Note, this is not currently a problem because all message handlers are
in-kernel but eventually we may e.g. get this exported to userspace.

Note also, that this is not a performance critical path: messages (unlike
events) represent rare events so it doesn't really matter (from performance
point of view) if we copy too much.

Fix the issue by taking into account the real message length. The temporary
buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 029378c27421..2b5572146358 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
 			return;
 
 		INIT_WORK(&ctx->work, vmbus_onmessage_work);
-		memcpy(&ctx->msg, msg, sizeof(*msg));
+		memcpy(&ctx->msg, msg, sizeof(msg->header) +
+		       msg->header.payload_size);
 
 		/*
 		 * The host can generate a rescind message while we
-- 
2.25.1

Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed

[163]

On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
> messages. Each message comes with a header (16 bytes) which specifies the
> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
> look at the real message length and copies the whole slot to a temporary
> buffer before passing it to message handlers. This is potentially dangerous
> as hypervisor doesn't have to clean the whole slot when putting a new
> message there and a message handler can get access to some data which
> belongs to a previous message.
> 
> Note, this is not currently a problem because all message handlers are
> in-kernel but eventually we may e.g. get this exported to userspace.
> 
> Note also, that this is not a performance critical path: messages (unlike
> events) represent rare events so it doesn't really matter (from performance
> point of view) if we copy too much.
> 
> Fix the issue by taking into account the real message length. The temporary
> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>   drivers/hv/vmbus_drv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 029378c27421..2b5572146358 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>   			return;
>   
>   		INIT_WORK(&ctx->work, vmbus_onmessage_work);
> -		memcpy(&ctx->msg, msg, sizeof(*msg));
> +		memcpy(&ctx->msg, msg, sizeof(msg->header) +
> +		       msg->header.payload_size);
>   

Hi Vitaly:
      I think we still need to check whether the payload_size passed from
Hyper-V is valid or not here to avoid cross-border issue before doing
copying.

Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed

[Vitaly Kuznetsov]

163 <freedomsky1986@163.com> writes:

> On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> messages. Each message comes with a header (16 bytes) which specifies the
>> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> look at the real message length and copies the whole slot to a temporary
>> buffer before passing it to message handlers. This is potentially dangerous
>> as hypervisor doesn't have to clean the whole slot when putting a new
>> message there and a message handler can get access to some data which
>> belongs to a previous message.
>> 
>> Note, this is not currently a problem because all message handlers are
>> in-kernel but eventually we may e.g. get this exported to userspace.
>> 
>> Note also, that this is not a performance critical path: messages (unlike
>> events) represent rare events so it doesn't really matter (from performance
>> point of view) if we copy too much.
>> 
>> Fix the issue by taking into account the real message length. The temporary
>> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>   drivers/hv/vmbus_drv.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 029378c27421..2b5572146358 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>>   			return;
>>   
>>   		INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> -		memcpy(&ctx->msg, msg, sizeof(*msg));
>> +		memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> +		       msg->header.payload_size);
>>   
>
> Hi Vitaly:
>       I think we still need to check whether the payload_size passed from
> Hyper-V is valid or not here to avoid cross-border issue before doing
> copying.

Sure,

the header.payload_size must be 0 <= header.payload_size <= 240

I'll add the check.

-- 
Vitaly

RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Thursday, April 2, 2020 9:27 AM
> 
> 163 <freedomsky1986@163.com> writes:
> 
> > On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
> >> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
> >> messages. Each message comes with a header (16 bytes) which specifies the
> >> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
> >> look at the real message length and copies the whole slot to a temporary
> >> buffer before passing it to message handlers. This is potentially dangerous
> >> as hypervisor doesn't have to clean the whole slot when putting a new
> >> message there and a message handler can get access to some data which
> >> belongs to a previous message.
> >>
> >> Note, this is not currently a problem because all message handlers are
> >> in-kernel but eventually we may e.g. get this exported to userspace.
> >>
> >> Note also, that this is not a performance critical path: messages (unlike
> >> events) represent rare events so it doesn't really matter (from performance
> >> point of view) if we copy too much.
> >>
> >> Fix the issue by taking into account the real message length. The temporary
> >> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
> >>
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>   drivers/hv/vmbus_drv.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> >> index 029378c27421..2b5572146358 100644
> >> --- a/drivers/hv/vmbus_drv.c
> >> +++ b/drivers/hv/vmbus_drv.c
> >> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
> >>   			return;
> >>
> >>   		INIT_WORK(&ctx->work, vmbus_onmessage_work);
> >> -		memcpy(&ctx->msg, msg, sizeof(*msg));
> >> +		memcpy(&ctx->msg, msg, sizeof(msg->header) +
> >> +		       msg->header.payload_size);
> >>
> >
> > Hi Vitaly:
> >       I think we still need to check whether the payload_size passed from
> > Hyper-V is valid or not here to avoid cross-border issue before doing
> > copying.
> 
> Sure,
> 
> the header.payload_size must be 0 <= header.payload_size <= 240
> 
> I'll add the check.
> 

With this change,

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

FWIW, all of this VMbus code, as well as the drivers for the VMbus
synthetic devices, make the fundamental assumption that Hyper-V
is trustworthy and doesn't send any malformed messages.  However,
starting this summer we will be submitting changes to "harden" all
of the interactions with Hyper-V to no longer make that assumption.
All relevant fields will be checked before being used so that incorrect
memory references aren't made.  This patch is one small step in that
direction. :-)

Michael

RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed

[Vitaly Kuznetsov]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Thursday, April 2, 2020 9:27 AM
>> 
>> 163 <freedomsky1986@163.com> writes:
>> 
>> > On 4/1/2020 6:36 PM, Vitaly Kuznetsov wrote:
>> >> Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
>> >> messages. Each message comes with a header (16 bytes) which specifies the
>> >> payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
>> >> look at the real message length and copies the whole slot to a temporary
>> >> buffer before passing it to message handlers. This is potentially dangerous
>> >> as hypervisor doesn't have to clean the whole slot when putting a new
>> >> message there and a message handler can get access to some data which
>> >> belongs to a previous message.
>> >>
>> >> Note, this is not currently a problem because all message handlers are
>> >> in-kernel but eventually we may e.g. get this exported to userspace.
>> >>
>> >> Note also, that this is not a performance critical path: messages (unlike
>> >> events) represent rare events so it doesn't really matter (from performance
>> >> point of view) if we copy too much.
>> >>
>> >> Fix the issue by taking into account the real message length. The temporary
>> >> buffer allocated by vmbus_on_msg_dpc() remains fixed size for now.
>> >>
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >> ---
>> >>   drivers/hv/vmbus_drv.c | 3 ++-
>> >>   1 file changed, 2 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> >> index 029378c27421..2b5572146358 100644
>> >> --- a/drivers/hv/vmbus_drv.c
>> >> +++ b/drivers/hv/vmbus_drv.c
>> >> @@ -1043,7 +1043,8 @@ void vmbus_on_msg_dpc(unsigned long data)
>> >>   			return;
>> >>
>> >>   		INIT_WORK(&ctx->work, vmbus_onmessage_work);
>> >> -		memcpy(&ctx->msg, msg, sizeof(*msg));
>> >> +		memcpy(&ctx->msg, msg, sizeof(msg->header) +
>> >> +		       msg->header.payload_size);
>> >>
>> >
>> > Hi Vitaly:
>> >       I think we still need to check whether the payload_size passed from
>> > Hyper-V is valid or not here to avoid cross-border issue before doing
>> > copying.
>> 
>> Sure,
>> 
>> the header.payload_size must be 0 <= header.payload_size <= 240
>> 
>> I'll add the check.
>> 
>
> With this change,
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
> FWIW, all of this VMbus code, as well as the drivers for the VMbus
> synthetic devices, make the fundamental assumption that Hyper-V
> is trustworthy and doesn't send any malformed messages.  However,
> starting this summer we will be submitting changes to "harden" all
> of the interactions with Hyper-V to no longer make that assumption.
> All relevant fields will be checked before being used so that incorrect
> memory references aren't made.  This patch is one small step in that
> direction. :-)

What about 'alternetive' Hyper-V implementations? :-)

On a more serious note, my understanding is: if your hypervisor
misbehaves it's 'game over' for you (as a guest), the only question is
how hard is it going to be to figure out what happened. In this
particular case if hypervisor send us garbase we probably want to fail
immediately and not try to handle it as a valid message.

Thank you for your review!

-- 
Vitaly

[PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages

[Vitaly Kuznetsov]

When we need to pass a buffer with Hyper-V message we don't need to always
allocate 256 bytes for the message: the real message length is known from
the header. Change 'struct onmessage_work_context' to make it possible to
not over-allocate.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 2b5572146358..642782bef863 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -991,7 +991,10 @@ static struct bus_type  hv_bus = {
 
 struct onmessage_work_context {
 	struct work_struct work;
-	struct hv_message msg;
+	struct {
+		struct hv_message_header header;
+		u8 payload[];
+	} msg;
 };
 
 static void vmbus_onmessage_work(struct work_struct *work)
@@ -1038,7 +1041,8 @@ void vmbus_on_msg_dpc(unsigned long data)
 		goto msg_handled;
 
 	if (entry->handler_type	== VMHT_BLOCKING) {
-		ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
+		ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
+			      GFP_ATOMIC);
 		if (ctx == NULL)
 			return;
 
@@ -1092,10 +1096,11 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
 	WARN_ON(!is_hvsock_channel(channel));
 
 	/*
-	 * sizeof(*ctx) is small and the allocation should really not fail,
+	 * Allocation size is small and the allocation should really not fail,
 	 * otherwise the state of the hv_sock connections ends up in limbo.
 	 */
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL | __GFP_NOFAIL);
+	ctx = kzalloc(sizeof(*ctx) + sizeof(*rescind),
+		      GFP_KERNEL | __GFP_NOFAIL);
 
 	/*
 	 * So far, these are not really used by Linux. Just set them to the
@@ -1105,7 +1110,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
 	ctx->msg.header.payload_size = sizeof(*rescind);
 
 	/* These values are actually used by Linux. */
-	rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.u.payload;
+	rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.payload;
 	rescind->header.msgtype = CHANNELMSG_RESCIND_CHANNELOFFER;
 	rescind->child_relid = channel->offermsg.child_relid;
 
-- 
2.25.1

RE: [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:37 AM
> 
> When we need to pass a buffer with Hyper-V message we don't need to always
> allocate 256 bytes for the message: the real message length is known from
> the header. Change 'struct onmessage_work_context' to make it possible to
> not over-allocate.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/vmbus_drv.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

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

[PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()

[Vitaly Kuznetsov]

vmbus_onmessage() doesn't need the header of the message, it only
uses it to get to the payload, we can pass the pointer to the
payload directly.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 7 +------
 drivers/hv/vmbus_drv.c    | 3 ++-
 include/linux/hyperv.h    | 2 +-
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0370364169c4..c6bcfee6ac99 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1360,13 +1360,8 @@ channel_message_table[CHANNELMSG_COUNT] = {
  *
  * This is invoked in the vmbus worker thread context.
  */
-void vmbus_onmessage(void *context)
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr)
 {
-	struct hv_message *msg = context;
-	struct vmbus_channel_message_header *hdr;
-
-	hdr = (struct vmbus_channel_message_header *)msg->u.payload;
-
 	trace_vmbus_on_message(hdr);
 
 	/*
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 642782bef863..0f7bbf952d89 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1007,7 +1007,8 @@ static void vmbus_onmessage_work(struct work_struct *work)
 
 	ctx = container_of(work, struct onmessage_work_context,
 			   work);
-	vmbus_onmessage(&ctx->msg);
+	vmbus_onmessage((struct vmbus_channel_message_header *)
+			&ctx->msg.payload);
 	kfree(ctx);
 }
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 692c89ccf5df..cbd24f4e68d1 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1017,7 +1017,7 @@ static inline void clear_low_latency_mode(struct vmbus_channel *c)
 	c->low_latency = false;
 }
 
-void vmbus_onmessage(void *context);
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr);
 
 int vmbus_request_offers(void);
 
-- 
2.25.1

RE: [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, April 1, 2020 3:37 AM
> 
> vmbus_onmessage() doesn't need the header of the message, it only
> uses it to get to the payload, we can pass the pointer to the
> payload directly.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/channel_mgmt.c | 7 +------
>  drivers/hv/vmbus_drv.c    | 3 ++-
>  include/linux/hyperv.h    | 2 +-
>  3 files changed, 4 insertions(+), 8 deletions(-)
> 

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

[PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly

[Vitaly Kuznetsov]

Strictly speaking, compiler is free to use something different from 'u32'
for 'enum vmbus_channel_message_type' (e.g. char) but it doesn't happen in
real life, just add a BUILD_BUG_ON() guardian.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0f7bbf952d89..d684cbee7ae6 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1023,6 +1023,13 @@ void vmbus_on_msg_dpc(unsigned long data)
 	struct onmessage_work_context *ctx;
 	u32 message_type = msg->header.message_type;
 
+	/*
+	 * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
+	 * it is being used in 'struct vmbus_channel_message_header' definition
+	 * which is supposed to match hypervisor ABI.
+	 */
+	BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
+
 	if (message_type == HVMSG_NONE)
 		/* no msg */
 		return;
-- 
2.25.1

[PATCH 5/5] Drivers: hv: check VMBus messages lengths

[Vitaly Kuznetsov]

VMBus message handlers (channel_message_table) receive a pointer to
'struct vmbus_channel_message_header' and cast it to a structure of their
choice, which is sometimes longer than the header. We, however, don't check
that the message is long enough so in case hypervisor screws up we'll be
accessing memory beyond what was allocated for temporary buffer.

Previously, we used to always allocate and copy 256 bytes from message page
to temporary buffer but this is hardly better: in case the message is
shorter than we expect we'll be trying to consume garbage as some real
data and no memory guarding technique will be able to identify an issue.

Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
and check against it in vmbus_on_msg_dpc(). Note, we can't require the
exact length as new hypervisor versions may add extra fields to messages,
we only check that the message is not shorter than we expect.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
 drivers/hv/hyperv_vmbus.h |  1 +
 drivers/hv/vmbus_drv.c    |  6 +++++
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c6bcfee6ac99..d4ccc9b203fa 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1329,30 +1329,36 @@ static void vmbus_onversion_response(
 /* Channel message dispatch table */
 const struct vmbus_channel_message_table_entry
 channel_message_table[CHANNELMSG_COUNT] = {
-	{ CHANNELMSG_INVALID,			0, NULL },
-	{ CHANNELMSG_OFFERCHANNEL,		0, vmbus_onoffer },
-	{ CHANNELMSG_RESCIND_CHANNELOFFER,	0, vmbus_onoffer_rescind },
-	{ CHANNELMSG_REQUESTOFFERS,		0, NULL },
-	{ CHANNELMSG_ALLOFFERS_DELIVERED,	1, vmbus_onoffers_delivered },
-	{ CHANNELMSG_OPENCHANNEL,		0, NULL },
-	{ CHANNELMSG_OPENCHANNEL_RESULT,	1, vmbus_onopen_result },
-	{ CHANNELMSG_CLOSECHANNEL,		0, NULL },
-	{ CHANNELMSG_GPADL_HEADER,		0, NULL },
-	{ CHANNELMSG_GPADL_BODY,		0, NULL },
-	{ CHANNELMSG_GPADL_CREATED,		1, vmbus_ongpadl_created },
-	{ CHANNELMSG_GPADL_TEARDOWN,		0, NULL },
-	{ CHANNELMSG_GPADL_TORNDOWN,		1, vmbus_ongpadl_torndown },
-	{ CHANNELMSG_RELID_RELEASED,		0, NULL },
-	{ CHANNELMSG_INITIATE_CONTACT,		0, NULL },
-	{ CHANNELMSG_VERSION_RESPONSE,		1, vmbus_onversion_response },
-	{ CHANNELMSG_UNLOAD,			0, NULL },
-	{ CHANNELMSG_UNLOAD_RESPONSE,		1, vmbus_unload_response },
-	{ CHANNELMSG_18,			0, NULL },
-	{ CHANNELMSG_19,			0, NULL },
-	{ CHANNELMSG_20,			0, NULL },
-	{ CHANNELMSG_TL_CONNECT_REQUEST,	0, NULL },
-	{ CHANNELMSG_22,			0, NULL },
-	{ CHANNELMSG_TL_CONNECT_RESULT,		0, NULL },
+	{ CHANNELMSG_INVALID,			0, NULL, 0},
+	{ CHANNELMSG_OFFERCHANNEL,		0, vmbus_onoffer,
+		sizeof(struct vmbus_channel_offer_channel)},
+	{ CHANNELMSG_RESCIND_CHANNELOFFER,	0, vmbus_onoffer_rescind,
+		sizeof(struct vmbus_channel_rescind_offer) },
+	{ CHANNELMSG_REQUESTOFFERS,		0, NULL, 0},
+	{ CHANNELMSG_ALLOFFERS_DELIVERED,	1, vmbus_onoffers_delivered, 0},
+	{ CHANNELMSG_OPENCHANNEL,		0, NULL, 0},
+	{ CHANNELMSG_OPENCHANNEL_RESULT,	1, vmbus_onopen_result,
+		sizeof(struct vmbus_channel_open_result)},
+	{ CHANNELMSG_CLOSECHANNEL,		0, NULL, 0},
+	{ CHANNELMSG_GPADL_HEADER,		0, NULL, 0},
+	{ CHANNELMSG_GPADL_BODY,		0, NULL, 0},
+	{ CHANNELMSG_GPADL_CREATED,		1, vmbus_ongpadl_created,
+		sizeof(struct vmbus_channel_gpadl_created)},
+	{ CHANNELMSG_GPADL_TEARDOWN,		0, NULL, 0},
+	{ CHANNELMSG_GPADL_TORNDOWN,		1, vmbus_ongpadl_torndown,
+		sizeof(struct vmbus_channel_gpadl_torndown) },
+	{ CHANNELMSG_RELID_RELEASED,		0, NULL, 0},
+	{ CHANNELMSG_INITIATE_CONTACT,		0, NULL, 0},
+	{ CHANNELMSG_VERSION_RESPONSE,		1, vmbus_onversion_response,
+		sizeof(struct vmbus_channel_version_response)},
+	{ CHANNELMSG_UNLOAD,			0, NULL, 0},
+	{ CHANNELMSG_UNLOAD_RESPONSE,		1, vmbus_unload_response, 0},
+	{ CHANNELMSG_18,			0, NULL, 0},
+	{ CHANNELMSG_19,			0, NULL, 0},
+	{ CHANNELMSG_20,			0, NULL, 0},
+	{ CHANNELMSG_TL_CONNECT_REQUEST,	0, NULL, 0},
+	{ CHANNELMSG_22,			0, NULL, 0},
+	{ CHANNELMSG_TL_CONNECT_RESULT,		0, NULL, 0},
 };
 
 /*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f5fa3b3c9baf..7fd66a4e2951 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -317,6 +317,7 @@ struct vmbus_channel_message_table_entry {
 	enum vmbus_channel_message_type message_type;
 	enum vmbus_message_handler_type handler_type;
 	void (*message_handler)(struct vmbus_channel_message_header *msg);
+	u32 min_payload_len;
 };
 
 extern const struct vmbus_channel_message_table_entry
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d684cbee7ae6..7b7229199936 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1048,6 +1048,12 @@ 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",
+			  hdr->msgtype, msg->header.payload_size);
+		goto msg_handled;
+	}
+
 	if (entry->handler_type	== VMHT_BLOCKING) {
 		ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
 			      GFP_ATOMIC);
-- 
2.25.1

RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, April 1, 2020 3:38 AM
> 
> VMBus message handlers (channel_message_table) receive a pointer to
> 'struct vmbus_channel_message_header' and cast it to a structure of their
> choice, which is sometimes longer than the header. We, however, don't check
> that the message is long enough so in case hypervisor screws up we'll be
> accessing memory beyond what was allocated for temporary buffer.
> 
> Previously, we used to always allocate and copy 256 bytes from message page
> to temporary buffer but this is hardly better: in case the message is
> shorter than we expect we'll be trying to consume garbage as some real
> data and no memory guarding technique will be able to identify an issue.
> 
> Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
> and check against it in vmbus_on_msg_dpc(). Note, we can't require the
> exact length as new hypervisor versions may add extra fields to messages,
> we only check that the message is not shorter than we expect.

This assumes that the current structure definitions don't already
include extra fields that were added in newer versions of Hyper-V.  If they did,
the minimum length test could fail on older versions of Hyper-V.  But I
looked through the structure definitions and don't see any indication that
such extra fields have been added, so this should be OK.

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
>  drivers/hv/hyperv_vmbus.h |  1 +
>  drivers/hv/vmbus_drv.c    |  6 +++++
>  3 files changed, 37 insertions(+), 24 deletions(-)
> 

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

RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths

[Vitaly Kuznetsov]

Michael Kelley <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, April 1, 2020 3:38 AM
>> 
>> VMBus message handlers (channel_message_table) receive a pointer to
>> 'struct vmbus_channel_message_header' and cast it to a structure of their
>> choice, which is sometimes longer than the header. We, however, don't check
>> that the message is long enough so in case hypervisor screws up we'll be
>> accessing memory beyond what was allocated for temporary buffer.
>> 
>> Previously, we used to always allocate and copy 256 bytes from message page
>> to temporary buffer but this is hardly better: in case the message is
>> shorter than we expect we'll be trying to consume garbage as some real
>> data and no memory guarding technique will be able to identify an issue.
>> 
>> Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
>> and check against it in vmbus_on_msg_dpc(). Note, we can't require the
>> exact length as new hypervisor versions may add extra fields to messages,
>> we only check that the message is not shorter than we expect.
>
> This assumes that the current structure definitions don't already
> include extra fields that were added in newer versions of Hyper-V.  If they did,
> the minimum length test could fail on older versions of Hyper-V.  But I
> looked through the structure definitions and don't see any indication that
> such extra fields have been added, so this should be OK.
>

Yes, my understanding as well. When we decide to extend some of these
structures for newer VMbus version we'll have a choice: keep the require
length minimal or implement a more somplex check (e.g. add a 'length
check' function pointer to vmbus_channel_message_table_entry() -- or
pass 'length' to all message handlers and handle it ther). But as we
currently have no such cases this will definitely look over-engineered
so I decided to go the easiest way.

>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
>>  drivers/hv/hyperv_vmbus.h |  1 +
>>  drivers/hv/vmbus_drv.c    |  6 +++++
>>  3 files changed, 37 insertions(+), 24 deletions(-)
>> 
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>

Thanks!

-- 
Vitaly

RE: [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, April 1, 2020 3:38 AM
> 
> Strictly speaking, compiler is free to use something different from 'u32'
> for 'enum vmbus_channel_message_type' (e.g. char) but it doesn't happen in
> real life, just add a BUILD_BUG_ON() guardian.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/vmbus_drv.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

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