From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
To: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Cc: "K . Y . Srinivasan" <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
Wei Liu <wei.liu@kernel.org>,
Michael Kelley <mikelley@microsoft.com>,
Juan Vazquez <juvazq@microsoft.com>,
Saruhan Karademir <skarade@microsoft.com>,
"Andrea Parri (Microsoft)" <parri.andrea@gmail.com>
Subject: [PATCH v2 7/7] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[]
Date: Wed, 2 Dec 2020 10:22:14 +0100 [thread overview]
Message-ID: <20201202092214.13520-8-parri.andrea@gmail.com> (raw)
In-Reply-To: <20201202092214.13520-1-parri.andrea@gmail.com>
Currently, vmbus_onoffer() and vmbus_process_offer() are not validating
whether a given entry in the vmbus_connection.channels[] array is empty
before filling the entry with a call of vmbus_channel_map_relid(). An
erroneous or malicious host could rely on this to leak channel objects.
Do not allow overwriting an entry vmbus_connection.channels[].
Reported-by: Juan Vazquez <juvazq@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
Changes since v1:
- Don't corrupt oldchannel if offer->child_relid is invalid
drivers/hv/channel_mgmt.c | 38 ++++++++++++++++++++++++--------------
drivers/hv/hyperv_vmbus.h | 2 +-
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..f91d476dfe381 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -354,10 +354,12 @@ static void free_channel(struct vmbus_channel *channel)
kobject_put(&channel->kobj);
}
-void vmbus_channel_map_relid(struct vmbus_channel *channel)
+int vmbus_channel_map_relid(struct vmbus_channel *channel)
{
- if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
- return;
+ u32 relid = channel->offermsg.child_relid;
+
+ if (WARN_ON(relid >= MAX_CHANNEL_RELIDS || vmbus_connection.channels[relid] != NULL))
+ return -EINVAL;
/*
* The mapping of the channel's relid is visible from the CPUs that
* execute vmbus_chan_sched() by the time that vmbus_chan_sched() will
@@ -383,18 +385,17 @@ void vmbus_channel_map_relid(struct vmbus_channel *channel)
* of the VMBus driver and vmbus_chan_sched() can not run before
* vmbus_bus_resume() has completed execution (cf. resume_noirq).
*/
- smp_store_mb(
- vmbus_connection.channels[channel->offermsg.child_relid],
- channel);
+ smp_store_mb(vmbus_connection.channels[relid], channel);
+ return 0;
}
void vmbus_channel_unmap_relid(struct vmbus_channel *channel)
{
- if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
+ u32 relid = channel->offermsg.child_relid;
+
+ if (WARN_ON(relid >= MAX_CHANNEL_RELIDS))
return;
- WRITE_ONCE(
- vmbus_connection.channels[channel->offermsg.child_relid],
- NULL);
+ WRITE_ONCE(vmbus_connection.channels[relid], NULL);
}
static void vmbus_release_relid(u32 relid)
@@ -601,6 +602,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
*/
atomic_dec(&vmbus_connection.offer_in_progress);
+ if (vmbus_channel_map_relid(newchannel)) {
+ mutex_unlock(&vmbus_connection.channel_mutex);
+ kfree(newchannel);
+ return;
+ }
+
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
if (guid_equal(&channel->offermsg.offer.if_type,
&newchannel->offermsg.offer.if_type) &&
@@ -619,6 +626,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
* Check to see if this is a valid sub-channel.
*/
if (newchannel->offermsg.offer.sub_channel_index == 0) {
+ vmbus_channel_unmap_relid(newchannel);
mutex_unlock(&vmbus_connection.channel_mutex);
/*
* Don't call free_channel(), because newchannel->kobj
@@ -635,8 +643,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
list_add_tail(&newchannel->sc_list, &channel->sc_list);
}
- vmbus_channel_map_relid(newchannel);
-
mutex_unlock(&vmbus_connection.channel_mutex);
cpus_read_unlock();
@@ -920,6 +926,8 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
+ u32 relid = offer->child_relid;
+
/*
* We're resuming from hibernation: all the sub-channel and
* hv_sock channels we had before the hibernation should have
@@ -954,8 +962,10 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
atomic_dec(&vmbus_connection.offer_in_progress);
WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
+ if (WARN_ON(vmbus_connection.channels[relid] != NULL))
+ return;
/* Fix up the relid. */
- oldchannel->offermsg.child_relid = offer->child_relid;
+ oldchannel->offermsg.child_relid = relid;
offer_sz = sizeof(*offer);
if (memcmp(offer, &oldchannel->offermsg, offer_sz) != 0) {
@@ -967,7 +977,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
* reoffers the device upon resume.
*/
pr_debug("vmbus offer changed: relid=%d\n",
- offer->child_relid);
+ relid);
print_hex_dump_debug("Old vmbus offer: ",
DUMP_PREFIX_OFFSET, 16, 4,
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 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
prev parent reply other threads:[~2020-12-02 9:24 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 9:22 [PATCH v2 0/7] Drivers: hv: vmbus: More VMBus-hardening changes Andrea Parri (Microsoft)
2020-12-02 9:22 ` [PATCH v2 1/7] Drivers: hv: vmbus: Initialize memory to be sent to the host Andrea Parri (Microsoft)
2020-12-02 9:22 ` [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc() Andrea Parri (Microsoft)
2020-12-02 12:22 ` Wei Liu
2020-12-02 13:37 ` Andrea Parri
2020-12-02 13:40 ` Wei Liu
2020-12-02 14:14 ` Andrea Parri
2020-12-02 9:22 ` [PATCH v2 3/7] Drivers: hv: vmbus: Avoid double fetch of payload_size " Andrea Parri (Microsoft)
2020-12-02 9:22 ` [PATCH v2 4/7] Drivers: hv: vmbus: Copy the hv_message object " Andrea Parri (Microsoft)
2020-12-06 18:39 ` Michael Kelley
2020-12-06 21:33 ` Andrea Parri
2020-12-02 9:22 ` [PATCH v2 5/7] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind() Andrea Parri (Microsoft)
2020-12-02 9:22 ` [PATCH v2 6/7] Drivers: hv: vmbus: Resolve race condition " Andrea Parri (Microsoft)
2020-12-02 9:22 ` Andrea Parri (Microsoft) [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201202092214.13520-8-parri.andrea@gmail.com \
--to=parri.andrea@gmail.com \
--cc=haiyangz@microsoft.com \
--cc=juvazq@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mikelley@microsoft.com \
--cc=skarade@microsoft.com \
--cc=sthemmin@microsoft.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is 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).