linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling
@ 2020-04-01 10:36 Vitaly Kuznetsov
  2020-04-01 10:36 ` [PATCH 1/5] Drivers: hv: copy from message page only what's needed Vitaly Kuznetsov
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] Drivers: hv: copy from message page only what's needed
  2020-04-01 10:36 [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling Vitaly Kuznetsov
@ 2020-04-01 10:36 ` Vitaly Kuznetsov
  2020-04-02 14:46   ` [EXTERNAL] " 163
  2020-04-01 10:36 ` [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages Vitaly Kuznetsov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages
  2020-04-01 10:36 [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling Vitaly Kuznetsov
  2020-04-01 10:36 ` [PATCH 1/5] Drivers: hv: copy from message page only what's needed Vitaly Kuznetsov
@ 2020-04-01 10:36 ` Vitaly Kuznetsov
  2020-04-03  1:54   ` Michael Kelley
  2020-04-01 10:36 ` [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage() Vitaly Kuznetsov
  2020-04-01 10:38 ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Vitaly Kuznetsov
  3 siblings, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
  2020-04-01 10:36 [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling Vitaly Kuznetsov
  2020-04-01 10:36 ` [PATCH 1/5] Drivers: hv: copy from message page only what's needed Vitaly Kuznetsov
  2020-04-01 10:36 ` [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages Vitaly Kuznetsov
@ 2020-04-01 10:36 ` Vitaly Kuznetsov
  2020-04-03  1:55   ` Michael Kelley
  2020-04-01 10:38 ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Vitaly Kuznetsov
  3 siblings, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:36 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly
  2020-04-01 10:36 [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling Vitaly Kuznetsov
                   ` (2 preceding siblings ...)
  2020-04-01 10:36 ` [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage() Vitaly Kuznetsov
@ 2020-04-01 10:38 ` Vitaly Kuznetsov
  2020-04-01 10:38   ` [PATCH 5/5] Drivers: hv: check VMBus messages lengths Vitaly Kuznetsov
  2020-04-03  1:56   ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Michael Kelley
  3 siblings, 2 replies; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:38 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] Drivers: hv: check VMBus messages lengths
  2020-04-01 10:38 ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Vitaly Kuznetsov
@ 2020-04-01 10:38   ` Vitaly Kuznetsov
  2020-04-03  2:00     ` Michael Kelley
  2020-04-03  1:56   ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Michael Kelley
  1 sibling, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-01 10:38 UTC (permalink / raw)
  To: linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
  2020-04-01 10:36 ` [PATCH 1/5] Drivers: hv: copy from message page only what's needed Vitaly Kuznetsov
@ 2020-04-02 14:46   ` 163
  2020-04-02 16:26     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 15+ messages in thread
From: 163 @ 2020-04-02 14:46 UTC (permalink / raw)
  To: Vitaly Kuznetsov, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
  2020-04-02 14:46   ` [EXTERNAL] " 163
@ 2020-04-02 16:26     ` Vitaly Kuznetsov
  2020-04-03  1:52       ` Michael Kelley
  0 siblings, 1 reply; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-02 16:26 UTC (permalink / raw)
  To: 163, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Andrea Parri (Microsoft)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
  2020-04-02 16:26     ` Vitaly Kuznetsov
@ 2020-04-03  1:52       ` Michael Kelley
  2020-04-03  7:03         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Kelley @ 2020-04-03  1:52 UTC (permalink / raw)
  To: vkuznets, 163, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages
  2020-04-01 10:36 ` [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages Vitaly Kuznetsov
@ 2020-04-03  1:54   ` Michael Kelley
  0 siblings, 0 replies; 15+ messages in thread
From: Michael Kelley @ 2020-04-03  1:54 UTC (permalink / raw)
  To: vkuznets, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
  2020-04-01 10:36 ` [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage() Vitaly Kuznetsov
@ 2020-04-03  1:55   ` Michael Kelley
  0 siblings, 0 replies; 15+ messages in thread
From: Michael Kelley @ 2020-04-03  1:55 UTC (permalink / raw)
  To: vkuznets, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly
  2020-04-01 10:38 ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Vitaly Kuznetsov
  2020-04-01 10:38   ` [PATCH 5/5] Drivers: hv: check VMBus messages lengths Vitaly Kuznetsov
@ 2020-04-03  1:56   ` Michael Kelley
  1 sibling, 0 replies; 15+ messages in thread
From: Michael Kelley @ 2020-04-03  1:56 UTC (permalink / raw)
  To: vkuznets, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths
  2020-04-01 10:38   ` [PATCH 5/5] Drivers: hv: check VMBus messages lengths Vitaly Kuznetsov
@ 2020-04-03  2:00     ` Michael Kelley
  2020-04-03  7:07       ` Vitaly Kuznetsov
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Kelley @ 2020-04-03  2:00 UTC (permalink / raw)
  To: vkuznets, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXTERNAL] [PATCH 1/5] Drivers: hv: copy from message page only what's needed
  2020-04-03  1:52       ` Michael Kelley
@ 2020-04-03  7:03         ` Vitaly Kuznetsov
  0 siblings, 0 replies; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-03  7:03 UTC (permalink / raw)
  To: Michael Kelley
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft),
	163, linux-hyperv

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH 5/5] Drivers: hv: check VMBus messages lengths
  2020-04-03  2:00     ` Michael Kelley
@ 2020-04-03  7:07       ` Vitaly Kuznetsov
  0 siblings, 0 replies; 15+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-03  7:07 UTC (permalink / raw)
  To: Michael Kelley, linux-hyperv
  Cc: linux-kernel, Wei Liu, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Andrea Parri (Microsoft)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-04-03  7:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 10:36 [PATCH 0/5] Drivers: hv: cleanup VMBus messages handling Vitaly Kuznetsov
2020-04-01 10:36 ` [PATCH 1/5] Drivers: hv: copy from message page only what's needed Vitaly Kuznetsov
2020-04-02 14:46   ` [EXTERNAL] " 163
2020-04-02 16:26     ` Vitaly Kuznetsov
2020-04-03  1:52       ` Michael Kelley
2020-04-03  7:03         ` Vitaly Kuznetsov
2020-04-01 10:36 ` [PATCH 2/5] Drivers: hv: allocate the exact needed memory for messages Vitaly Kuznetsov
2020-04-03  1:54   ` Michael Kelley
2020-04-01 10:36 ` [PATCH 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage() Vitaly Kuznetsov
2020-04-03  1:55   ` Michael Kelley
2020-04-01 10:38 ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Vitaly Kuznetsov
2020-04-01 10:38   ` [PATCH 5/5] Drivers: hv: check VMBus messages lengths Vitaly Kuznetsov
2020-04-03  2:00     ` Michael Kelley
2020-04-03  7:07       ` Vitaly Kuznetsov
2020-04-03  1:56   ` [PATCH 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly Michael Kelley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).