Re: [PATCH 2/2] riscv: Fix text patching when IPI are used
On Wed, Feb 28, 2024 at 06:51:49PM +0100, Alexandre Ghiti wrote: > For now, we use stop_machine() to patch the text and when we use IPIs for > remote icache flushes (which is emitted in patch_text_nosync()), the system > hangs. > > So instead, make sure every cpu executes the stop_machine() patching > function and emit a local icache flush there. > > Co-developed-by: Björn Töpel > Signed-off-by: Björn Töpel > Signed-off-by: Alexandre Ghiti Modulo the removal of the hunk discussed with Samuel, Reviewed-by: Andrea Parri Some nits / amendments to the inline comments below: > + /* > + * Make sure the patching store is effective *before* we > + * increment the counter which releases all waiting cpus > + * by using the release version of atomic increment. > + */ s/cpus/CPUs s/release version/release variant The comment could be amended with a description of the matching barrier(s), say, "The release pairs with the call to local_flush_icache_all() on the waiting CPU". (Same for the comment in patch_text_cb().) Andrea
Re: [PATCH 1/2] riscv: Remove superfluous smp_mb()
On Wed, Feb 28, 2024 at 06:51:48PM +0100, Alexandre Ghiti wrote: > This memory barrier is not needed and not documented so simply remove > it. > > Suggested-by: Andrea Parri > Signed-off-by: Alexandre Ghiti Reviewed-by: Andrea Parri Andrea
Re: [PATCH] riscv: Fix text patching when icache flushes use IPIs
> I did not even think of that, and it actually makes sense so I'll go
> with what you propose: I'll replace atomic_inc() with
> atomic_inc_return_release(). And I'll add the following comment if
> that's ok with you:
>
> "Make sure the patching store is effective *before* we increment the
> counter which releases all waiting cpus"
Yes, this sounds good to me.
> Honestly, I looked at it one minute, did not understand its purpose
> and said to myself "ok that can't hurt anyway, I may be missing
> something".
>
> FWIW, I see that arm64 uses isb() here. If you don't see its purpose,
> I'll remove it (here and where I copied it).
Removing the smp_mb() (and keeping the local_flush_icache_all()) seems
fine to me; thanks for the confirmation.
> > On a last topic, although somehow orthogonal to the scope of this patch,
> > I'm not sure the patch_{map,unmap}() dance in our patch_insn_write() is
> > correct: I can see why we may want (need to do) the local TLB flush be-
> > fore returning from patch_{map,unmap}(), but does a local flush suffice?
> > For comparison, arm64 seems to go through a complete dsb-tlbi-dsb(-isb)
> > sequence in their unmapping stage (and apparently relying on "no caching
> > of invalid ptes" in their mapping stage). Of course, "broadcasting" our
> > (riscv's) TLB invalidations will necessary introduce some complexity...
> >
> > Thoughts?
>
> To avoid remote TLBI, could we simply disable the preemption before
> the first patch_map()? arm64 disables the irqs, but that seems
> overkill to me, but maybe I'm missing something again?
Mmh, I'm afraid this will require more thinking/probing on my end (not
really "the expert" of the codebase at stake...). Maybe the ftrace
reviewers will provide further ideas/suggestions for us to brainstorm.
Andrea
Re: [PATCH] riscv: Fix text patching when icache flushes use IPIs
> +static int __ftrace_modify_code(void *data)
> +{
> + struct ftrace_modify_param *param = data;
> +
> + if (atomic_inc_return(>cpu_count) == num_online_cpus()) {
> + ftrace_modify_all_code(param->command);
> + atomic_inc(>cpu_count);
I stared at ftrace_modify_all_code() for a bit but honestly I don't see
what prevents the ->cpu_count increment from being reordered before the
insn write(s) (architecturally) now that you have removed the IPI dance:
perhaps add an smp_wmb() right before the atomic_inc() (or promote this
latter to a (void)atomic_inc_return_release()) and/or an inline comment
saying why such reordering is not possible?
> + } else {
> + while (atomic_read(>cpu_count) <= num_online_cpus())
> + cpu_relax();
> + smp_mb();
I see that you've lifted/copied the memory barrier from patch_text_cb():
what's its point? AFAIU, the barrier has no ordering effect on program
order later insn fetches; perhaps the code was based on some legacy/old
version of Zifencei? IAC, comments, comments, ... or maybe just remove
that memory barrier?
> + }
> +
> + local_flush_icache_all();
> +
> + return 0;
> +}
[...]
> @@ -232,8 +230,7 @@ static int patch_text_cb(void *data)
> if (atomic_inc_return(>cpu_count) == num_online_cpus()) {
> for (i = 0; ret == 0 && i < patch->ninsns; i++) {
> len = GET_INSN_LENGTH(patch->insns[i]);
> - ret = patch_text_nosync(patch->addr + i * len,
> - >insns[i], len);
> + ret = patch_insn_write(patch->addr + i * len,
> >insns[i], len);
> }
> atomic_inc(>cpu_count);
> } else {
> @@ -242,6 +239,8 @@ static int patch_text_cb(void *data)
> smp_mb();
> }
>
> + local_flush_icache_all();
> +
> return ret;
> }
> NOKPROBE_SYMBOL(patch_text_cb);
My above remarks/questions also apply to this function.
On a last topic, although somehow orthogonal to the scope of this patch,
I'm not sure the patch_{map,unmap}() dance in our patch_insn_write() is
correct: I can see why we may want (need to do) the local TLB flush be-
fore returning from patch_{map,unmap}(), but does a local flush suffice?
For comparison, arm64 seems to go through a complete dsb-tlbi-dsb(-isb)
sequence in their unmapping stage (and apparently relying on "no caching
of invalid ptes" in their mapping stage). Of course, "broadcasting" our
(riscv's) TLB invalidations will necessary introduce some complexity...
Thoughts?
Andrea
[PATCH v2] Drivers: hv: vmbus: Initialize unload_event statically
If a malicious or compromised Hyper-V sends a spurious message of type
CHANNELMSG_UNLOAD_RESPONSE, the function vmbus_unload_response() will
call complete() on an uninitialized event, and cause an oops.
Reported-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since v1[1]:
- add inline comment in vmbus_unload_response()
[1] https://lkml.kernel.org/r/20210416143932.16512-1-parri.and...@gmail.com
drivers/hv/channel_mgmt.c | 7 ++-
drivers/hv/connection.c | 2 ++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 4c9e45d1f462c..335a10ee03a5e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -826,6 +826,11 @@ static void vmbus_unload_response(struct
vmbus_channel_message_header *hdr)
/*
* This is a global event; just wakeup the waiting thread.
* Once we successfully unload, we can cleanup the monitor state.
+*
+* NB. A malicious or compromised Hyper-V could send a spurious
+* message of type CHANNELMSG_UNLOAD_RESPONSE, and trigger a call
+* of the complete() below. Make sure that unload_event has been
+* initialized by the time this complete() is executed.
*/
complete(_connection.unload_event);
}
@@ -841,7 +846,7 @@ void vmbus_initiate_unload(bool crash)
if (vmbus_proto_version < VERSION_WIN8_1)
return;
- init_completion(_connection.unload_event);
+ reinit_completion(_connection.unload_event);
memset(, 0, sizeof(struct vmbus_channel_message_header));
hdr.msgtype = CHANNELMSG_UNLOAD;
vmbus_post_msg(, sizeof(struct vmbus_channel_message_header),
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index dc19d5ae4373c..311cd005b3be6 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -26,6 +26,8 @@
struct vmbus_connection vmbus_connection = {
.conn_state = DISCONNECTED,
+ .unload_event = COMPLETION_INITIALIZER(
+ vmbus_connection.unload_event),
.next_gpadl_handle = ATOMIC_INIT(0xE1E10),
.ready_for_suspend_event = COMPLETION_INITIALIZER(
--
2.25.1
Re: [PATCH] Drivers: hv: vmbus: Initialize unload_event statically
On Fri, Apr 16, 2021 at 03:25:03PM +, Michael Kelley wrote: > From: Andrea Parri (Microsoft) Sent: Friday, April > 16, 2021 7:40 AM > > > > If a malicious or compromised Hyper-V sends a spurious message of type > > CHANNELMSG_UNLOAD_RESPONSE, the function vmbus_unload_response() will > > call complete() on an uninitialized event, and cause an oops. > > Please leave a comment somewhere in the code itself that describes this > scenario so that somebody in the future doesn't decide it's OK to "simplify" > the > initialization of unload_event. :-) Yes, will do for v2. Thanks, Andrea
[PATCH] Drivers: hv: vmbus: Initialize unload_event statically
If a malicious or compromised Hyper-V sends a spurious message of type
CHANNELMSG_UNLOAD_RESPONSE, the function vmbus_unload_response() will
call complete() on an uninitialized event, and cause an oops.
Reported-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel_mgmt.c | 2 +-
drivers/hv/connection.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index f3cf4af01e102..1efb616480a64 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -841,7 +841,7 @@ void vmbus_initiate_unload(bool crash)
if (vmbus_proto_version < VERSION_WIN8_1)
return;
- init_completion(_connection.unload_event);
+ reinit_completion(_connection.unload_event);
memset(, 0, sizeof(struct vmbus_channel_message_header));
hdr.msgtype = CHANNELMSG_UNLOAD;
vmbus_post_msg(, sizeof(struct vmbus_channel_message_header),
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 350e8c5cafa8c..529dcc47f3e11 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -26,6 +26,8 @@
struct vmbus_connection vmbus_connection = {
.conn_state = DISCONNECTED,
+ .unload_event = COMPLETION_INITIALIZER(
+ vmbus_connection.unload_event),
.next_gpadl_handle = ATOMIC_INIT(0xE1E10),
.ready_for_suspend_event = COMPLETION_INITIALIZER(
--
2.25.1
[PATCH v3 3/3] Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline
Check that enough time has passed such that the modify channel message
has been processed before taking a CPU offline.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/hv.c | 56 ++---
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 3e6ff83adff42..e0c522d143a37 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -292,12 +293,50 @@ void hv_synic_disable_regs(unsigned int cpu)
disable_percpu_irq(vmbus_irq);
}
+#define HV_MAX_TRIES 3
+/*
+ * Scan the event flags page of 'this' CPU looking for any bit that is set.
If we find one
+ * bit set, then wait for a few milliseconds. Repeat these steps for a
maximum of 3 times.
+ * Return 'true', if there is still any set bit after this operation; 'false',
otherwise.
+ *
+ * If a bit is set, that means there is a pending channel interrupt. The
expectation is
+ * that the normal interrupt handling mechanism will find and process the
channel interrupt
+ * "very soon", and in the process clear the bit.
+ */
+static bool hv_synic_event_pending(void)
+{
+ struct hv_per_cpu_context *hv_cpu =
this_cpu_ptr(hv_context.cpu_context);
+ union hv_synic_event_flags *event =
+ (union hv_synic_event_flags *)hv_cpu->synic_event_page +
VMBUS_MESSAGE_SINT;
+ unsigned long *recv_int_page = event->flags; /* assumes VMBus version
>= VERSION_WIN8 */
+ bool pending;
+ u32 relid;
+ int tries = 0;
+
+retry:
+ pending = false;
+ for_each_set_bit(relid, recv_int_page, HV_EVENT_FLAGS_COUNT) {
+ /* Special case - VMBus channel protocol messages */
+ if (relid == 0)
+ continue;
+ pending = true;
+ break;
+ }
+ if (pending && tries++ < HV_MAX_TRIES) {
+ usleep_range(1, 2);
+ goto retry;
+ }
+ return pending;
+}
int hv_synic_cleanup(unsigned int cpu)
{
struct vmbus_channel *channel, *sc;
bool channel_found = false;
+ if (vmbus_connection.conn_state != CONNECTED)
+ goto always_cleanup;
+
/*
* Hyper-V does not provide a way to change the connect CPU once
* it is set; we must prevent the connect CPU from going offline
@@ -305,8 +344,7 @@ int hv_synic_cleanup(unsigned int cpu)
* path where the vmbus is already disconnected, the CPU must be
* allowed to shut down.
*/
- if (cpu == VMBUS_CONNECT_CPU &&
- vmbus_connection.conn_state == CONNECTED)
+ if (cpu == VMBUS_CONNECT_CPU)
return -EBUSY;
/*
@@ -333,9 +371,21 @@ int hv_synic_cleanup(unsigned int cpu)
}
mutex_unlock(_connection.channel_mutex);
- if (channel_found && vmbus_connection.conn_state == CONNECTED)
+ if (channel_found)
+ return -EBUSY;
+
+ /*
+* channel_found == false means that any channels that were previously
+* assigned to the CPU have been reassigned elsewhere with a call of
+* vmbus_send_modifychannel(). Scan the event flags page looking for
+* bits that are set and waiting with a timeout for vmbus_chan_sched()
+* to process such bits. If bits are still set after this operation
+* and VMBus is connected, fail the CPU offlining operation.
+*/
+ if (vmbus_proto_version >= VERSION_WIN10_V4_1 &&
hv_synic_event_pending())
return -EBUSY;
+always_cleanup:
hv_stimer_legacy_cleanup(cpu);
hv_synic_disable_regs(cpu);
--
2.25.1
[PATCH v3 2/3] Drivers: hv: vmbus: Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE
Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE message type, and code
to receive and process such a message.
Signed-off-by: Andrea Parri (Microsoft)
Reviewed-by: Michael Kelley
---
drivers/hv/channel.c | 99 ---
drivers/hv/channel_mgmt.c | 42 +
drivers/hv/hv_trace.h | 15 ++
drivers/hv/vmbus_drv.c| 4 +-
include/linux/hyperv.h| 11 -
5 files changed, 152 insertions(+), 19 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9ccea..aa4ef75d8dee2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -209,31 +209,96 @@ int vmbus_send_tl_connect_request(const guid_t
*shv_guest_servie_id,
}
EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
+static int send_modifychannel_without_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel msg;
+ int ret;
+
+ memset(, 0, sizeof(msg));
+ msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg.child_relid = channel->offermsg.child_relid;
+ msg.target_vp = target_vp;
+
+ ret = vmbus_post_msg(, sizeof(msg), true);
+ trace_vmbus_send_modifychannel(, ret);
+
+ return ret;
+}
+
+static int send_modifychannel_with_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel *msg;
+ struct vmbus_channel_msginfo *info;
+ unsigned long flags;
+ int ret;
+
+ info = kzalloc(sizeof(struct vmbus_channel_msginfo) +
+ sizeof(struct vmbus_channel_modifychannel),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ init_completion(>waitevent);
+ info->waiting_channel = channel;
+
+ msg = (struct vmbus_channel_modifychannel *)info->msg;
+ msg->header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg->child_relid = channel->offermsg.child_relid;
+ msg->target_vp = target_vp;
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_add_tail(>msglistentry, _connection.chn_msg_list);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ ret = vmbus_post_msg(msg, sizeof(*msg), true);
+ trace_vmbus_send_modifychannel(msg, ret);
+ if (ret != 0) {
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock,
flags);
+ goto free_info;
+ }
+
+ /*
+* Release channel_mutex; otherwise, vmbus_onoffer_rescind() could
block on
+* the mutex and be unable to signal the completion.
+*
+* See the caller target_cpu_store() for information about the usage of
the
+* mutex.
+*/
+ mutex_unlock(_connection.channel_mutex);
+ wait_for_completion(>waitevent);
+ mutex_lock(_connection.channel_mutex);
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ if (info->response.modify_response.status)
+ ret = -EAGAIN;
+
+free_info:
+ kfree(info);
+ return ret;
+}
+
/*
* Set/change the vCPU (@target_vp) the channel (@child_relid) will interrupt.
*
- * CHANNELMSG_MODIFYCHANNEL messages are aynchronous. Also, Hyper-V does not
- * ACK such messages. IOW we can't know when the host will stop interrupting
- * the "old" vCPU and start interrupting the "new" vCPU for the given channel.
+ * CHANNELMSG_MODIFYCHANNEL messages are aynchronous. When VMbus version 5.3
+ * or later is negotiated, Hyper-V always sends an ACK in response to such a
+ * message. For VMbus version 5.2 and earlier, it never sends an ACK. With-
+ * out an ACK, we can not know when the host will stop interrupting the "old"
+ * vCPU and start interrupting the "new" vCPU for the given channel.
*
* The CHANNELMSG_MODIFYCHANNEL message type is supported since VMBus version
* VERSION_WIN10_V4_1.
*/
-int vmbus_send_modifychannel(u32 child_relid, u32 target_vp)
+int vmbus_send_modifychannel(struct vmbus_channel *channel, u32 target_vp)
{
- struct vmbus_channel_modifychannel conn_msg;
- int ret;
-
- memset(_msg, 0, sizeof(conn_msg));
- conn_msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
- conn_msg.child_relid = child_relid;
- conn_msg.target_vp = target_vp;
-
- ret = vmbus_post_msg(_msg, sizeof(conn_msg), true);
-
- trace_vmbus_send_modifychannel(_msg, ret);
-
- return ret;
+ if (vmbus_proto_version >= VERSION_WIN10_V5_3)
+ return send_modifychannel_with_ack(channel, target_vp);
+ return send_modifychannel_without_ack(channel, target_vp);
}
EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
diff --git a/drivers/hv/ch
[PATCH v3 1/3] Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3
Hyper-V has added VMBus protocol version 5.3. Allow Linux guests to
negotiate the new version on version of Hyper-V that support it.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/connection.c | 3 ++-
include/linux/hyperv.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 350e8c5cafa8c..dc19d5ae4373c 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -45,6 +45,7 @@ EXPORT_SYMBOL_GPL(vmbus_proto_version);
* Table of VMBus versions listed from newest to oldest.
*/
static __u32 vmbus_versions[] = {
+ VERSION_WIN10_V5_3,
VERSION_WIN10_V5_2,
VERSION_WIN10_V5_1,
VERSION_WIN10_V5,
@@ -60,7 +61,7 @@ static __u32 vmbus_versions[] = {
* Maximal VMBus protocol version guests can negotiate. Useful to cap the
* VMBus version for testing and debugging purpose.
*/
-static uint max_version = VERSION_WIN10_V5_2;
+static uint max_version = VERSION_WIN10_V5_3;
module_param(max_version, uint, S_IRUGO);
MODULE_PARM_DESC(max_version,
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 2c18c8e768efe..3ce36bbb398e9 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -234,6 +234,7 @@ static inline u32 hv_get_avail_to_write_percent(
* 5 . 0 (Newer Windows 10)
* 5 . 1 (Windows 10 RS4)
* 5 . 2 (Windows Server 2019, RS5)
+ * 5 . 3 (Windows Server 2022)
*/
#define VERSION_WS2008 ((0 << 16) | (13))
@@ -245,6 +246,7 @@ static inline u32 hv_get_avail_to_write_percent(
#define VERSION_WIN10_V5 ((5 << 16) | (0))
#define VERSION_WIN10_V5_1 ((5 << 16) | (1))
#define VERSION_WIN10_V5_2 ((5 << 16) | (2))
+#define VERSION_WIN10_V5_3 ((5 << 16) | (3))
/* Make maximum size of pipe payload of 16K */
#define MAX_PIPE_DATA_PAYLOAD (sizeof(u8) * 16384)
--
2.25.1
[PATCH v3 0/3] Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE
Changes since v2[1]: - fix VMbus protocol version name - add Reviewed-by: tag - refactor/simplyfy changes in hv_synic_cleanup() Changes since v1[2]: - rebase on hyperv-next - split changes into three patches - fix error handling in send_modifychannel_with_ack() - remove rescind checks in send_modifychannel_with_ack() - remove unneeded test in hv_synic_event_pending() - add/amend inline comments - style changes [1] https://lkml.kernel.org/r/20210414150118.2843-1-parri.and...@gmail.com [2] https://lkml.kernel.org/r/20201126191210.13115-1-parri.and...@gmail.com Andrea Parri (Microsoft) (3): Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3 Drivers: hv: vmbus: Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline drivers/hv/channel.c | 99 --- drivers/hv/channel_mgmt.c | 42 + drivers/hv/connection.c | 3 +- drivers/hv/hv.c | 56 -- drivers/hv/hv_trace.h | 15 ++ drivers/hv/vmbus_drv.c| 4 +- include/linux/hyperv.h| 13 - 7 files changed, 209 insertions(+), 23 deletions(-) -- 2.25.1
Re: [PATCH v2 3/3] Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline
> > @@ -336,6 +372,19 @@ int hv_synic_cleanup(unsigned int cpu)
> > if (channel_found && vmbus_connection.conn_state == CONNECTED)
> > return -EBUSY;
> >
> > + if (vmbus_proto_version >= VERSION_WIN10_V4_1) {
> > + /*
> > +* channel_found == false means that any channels that were
> > previously
> > +* assigned to the CPU have been reassigned elsewhere with a
> > call of
> > +* vmbus_send_modifychannel(). Scan the event flags page
> > looking for
> > +* bits that are set and waiting with a timeout for
> > vmbus_chan_sched()
> > +* to process such bits. If bits are still set after this
> > operation
> > +* and VMBus is connected, fail the CPU offlining operation.
> > +*/
> > + if (hv_synic_event_pending() && vmbus_connection.conn_state ==
> > CONNECTED)
> > + return -EBUSY;
> > + }
> > +
>
> Perhaps the test for conn_state == CONNECTED could be factored out as
> follows. If we're
> not CONNECTED (i.e., in the panic or kexec path) we should be able to also
> skip the search
> for channels that are bound to the CPU since we will ignore the result anyway.
>
> if (vmbus_connection.conn_state != CONNECTED)
> goto always_cleanup;
>
> if (cpu == VMBUS_CONNECT_CPU)
> return -EBUSY;
>
> [Code to search for channels that are bound to the CPU we're about to
> clean up]
>
> if (channel_found)
> return -EBUSY;
>
> /*
>* channel_found == false means that any channels that were previously
>* assigned to the CPU have been reassigned elsewhere with a call of
>* vmbus_send_modifychannel(). Scan the event flags page looking for
>* bits that are set and waiting with a timeout for vmbus_chan_sched()
>* to process such bits. If bits are still set after this operation
>* and VMBus is connected, fail the CPU offlining operation.
>*/
> if (vmbus_proto_version >= VERSION_WIN10_V4_1 &&
> hv_synic_event_pending())
> return -EBUSY;
>
> always_cleanup:
Agreed, applied. Thank you for the suggestion,
Andrea
Re: [PATCH v2 1/3] Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3
> > @@ -234,6 +234,7 @@ static inline u32 hv_get_avail_to_write_percent( > > * 5 . 0 (Newer Windows 10) > > * 5 . 1 (Windows 10 RS4) > > * 5 . 2 (Windows Server 2019, RS5) > > + * 5 . 3 (Windows Server 2021) // FIXME: use proper version number/name > > The official name is now public information as "Windows Server 2022". Thank you, I've updated the name and removed the FIXME. Andrea
[PATCH hyperv-next] scsi: storvsc: Use blk_mq_unique_tag() to generate requestIDs
Use blk_mq_unique_tag() to generate requestIDs for StorVSC, avoiding
all issues with allocating enough entries in the VMbus requestor.
Suggested-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since RFC:
- pass sentinel values for {init,reset}_request in vmbus_sendpacket()
- remove/inline the storvsc_request_addr() callback
- make storvsc_next_request_id() 'static'
- add code to handle the case of an unsolicited message from Hyper-V
- other minor/style changes
[1] https://lkml.kernel.org/r/20210408161315.341888-1-parri.and...@gmail.com
drivers/hv/channel.c | 14 ++---
drivers/hv/ring_buffer.c | 13 +++--
drivers/net/hyperv/netvsc.c | 8 ++-
drivers/net/hyperv/rndis_filter.c | 2 +
drivers/scsi/storvsc_drv.c| 94 +--
include/linux/hyperv.h| 13 -
6 files changed, 95 insertions(+), 49 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9ccea..f78e02ace51e8 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -1121,15 +1121,14 @@ EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
* vmbus_next_request_id - Returns a new request id. It is also
* the index at which the guest memory address is stored.
* Uses a spin lock to avoid race conditions.
- * @rqstor: Pointer to the requestor struct
+ * @channel: Pointer to the VMbus channel struct
* @rqst_add: Guest memory address to be stored in the array
*/
-u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
+u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr)
{
+ struct vmbus_requestor *rqstor = >requestor;
unsigned long flags;
u64 current_id;
- const struct vmbus_channel *channel =
- container_of(rqstor, const struct vmbus_channel, requestor);
/* Check rqstor has been initialized */
if (!channel->rqstor_size)
@@ -1163,16 +1162,15 @@ EXPORT_SYMBOL_GPL(vmbus_next_request_id);
/*
* vmbus_request_addr - Returns the memory address stored at @trans_id
* in @rqstor. Uses a spin lock to avoid race conditions.
- * @rqstor: Pointer to the requestor struct
+ * @channel: Pointer to the VMbus channel struct
* @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
* next request id.
*/
-u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
+u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id)
{
+ struct vmbus_requestor *rqstor = >requestor;
unsigned long flags;
u64 req_addr;
- const struct vmbus_channel *channel =
- container_of(rqstor, const struct vmbus_channel, requestor);
/* Check rqstor has been initialized */
if (!channel->rqstor_size)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index ecd82ebfd5bc4..2bf57677272b5 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -310,10 +310,12 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
*/
if (desc->flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) {
- rqst_id = vmbus_next_request_id(>requestor, requestid);
- if (rqst_id == VMBUS_RQST_ERROR) {
- spin_unlock_irqrestore(_info->ring_lock, flags);
- return -EAGAIN;
+ if (channel->next_request_id_callback != NULL) {
+ rqst_id = channel->next_request_id_callback(channel,
requestid);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+
spin_unlock_irqrestore(_info->ring_lock, flags);
+ return -EAGAIN;
+ }
}
}
desc = hv_get_ring_buffer(outring_info) + old_write;
@@ -341,7 +343,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
if (channel->rescind) {
if (rqst_id != VMBUS_NO_RQSTOR) {
/* Reclaim request ID to avoid leak of IDs */
- vmbus_request_addr(>requestor, rqst_id);
+ if (channel->request_addr_callback != NULL)
+ channel->request_addr_callback(channel,
rqst_id);
}
return -ENODEV;
}
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index c64cc7639c39c..1a221ce2d6fdc 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -730,7 +730,7 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
int queue_sends;
u64 cmd_rqst;
- cmd_rqst = vmbus_request_addr(>requestor, (u64)desc->trans_id);
+ cmd_rqst = channel->request_addr_callback(channel, (u64)desc->trans_id);
if (cmd_rqst == VMBUS_RQST_ERROR) {
netdev_err(ndev, "Incorrect transaction id\n");
return;
@@ -790,
[PATCH v2 3/3] Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline
Check that enough time has passed such that the modify channel message
has been processed before taking a CPU offline.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/hv.c | 49 +
1 file changed, 49 insertions(+)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 3e6ff83adff42..dc9aa1130b22f 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -292,6 +293,41 @@ void hv_synic_disable_regs(unsigned int cpu)
disable_percpu_irq(vmbus_irq);
}
+#define HV_MAX_TRIES 3
+/*
+ * Scan the event flags page of 'this' CPU looking for any bit that is set.
If we find one
+ * bit set, then wait for a few milliseconds. Repeat these steps for a
maximum of 3 times.
+ * Return 'true', if there is still any set bit after this operation; 'false',
otherwise.
+ *
+ * If a bit is set, that means there is a pending channel interrupt. The
expectation is
+ * that the normal interrupt handling mechanism will find and process the
channel interrupt
+ * "very soon", and in the process clear the bit.
+ */
+static bool hv_synic_event_pending(void)
+{
+ struct hv_per_cpu_context *hv_cpu =
this_cpu_ptr(hv_context.cpu_context);
+ union hv_synic_event_flags *event =
+ (union hv_synic_event_flags *)hv_cpu->synic_event_page +
VMBUS_MESSAGE_SINT;
+ unsigned long *recv_int_page = event->flags; /* assumes VMBus version
>= VERSION_WIN8 */
+ bool pending;
+ u32 relid;
+ int tries = 0;
+
+retry:
+ pending = false;
+ for_each_set_bit(relid, recv_int_page, HV_EVENT_FLAGS_COUNT) {
+ /* Special case - VMBus channel protocol messages */
+ if (relid == 0)
+ continue;
+ pending = true;
+ break;
+ }
+ if (pending && tries++ < HV_MAX_TRIES) {
+ usleep_range(1, 2);
+ goto retry;
+ }
+ return pending;
+}
int hv_synic_cleanup(unsigned int cpu)
{
@@ -336,6 +372,19 @@ int hv_synic_cleanup(unsigned int cpu)
if (channel_found && vmbus_connection.conn_state == CONNECTED)
return -EBUSY;
+ if (vmbus_proto_version >= VERSION_WIN10_V4_1) {
+ /*
+* channel_found == false means that any channels that were
previously
+* assigned to the CPU have been reassigned elsewhere with a
call of
+* vmbus_send_modifychannel(). Scan the event flags page
looking for
+* bits that are set and waiting with a timeout for
vmbus_chan_sched()
+* to process such bits. If bits are still set after this
operation
+* and VMBus is connected, fail the CPU offlining operation.
+*/
+ if (hv_synic_event_pending() && vmbus_connection.conn_state ==
CONNECTED)
+ return -EBUSY;
+ }
+
hv_stimer_legacy_cleanup(cpu);
hv_synic_disable_regs(cpu);
--
2.25.1
[PATCH v2 2/3] Drivers: hv: vmbus: Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE
Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE message type, and code
to receive and process such a message.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel.c | 99 ---
drivers/hv/channel_mgmt.c | 42 +
drivers/hv/hv_trace.h | 15 ++
drivers/hv/vmbus_drv.c| 4 +-
include/linux/hyperv.h| 11 -
5 files changed, 152 insertions(+), 19 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9ccea..aa4ef75d8dee2 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -209,31 +209,96 @@ int vmbus_send_tl_connect_request(const guid_t
*shv_guest_servie_id,
}
EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
+static int send_modifychannel_without_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel msg;
+ int ret;
+
+ memset(, 0, sizeof(msg));
+ msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg.child_relid = channel->offermsg.child_relid;
+ msg.target_vp = target_vp;
+
+ ret = vmbus_post_msg(, sizeof(msg), true);
+ trace_vmbus_send_modifychannel(, ret);
+
+ return ret;
+}
+
+static int send_modifychannel_with_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel *msg;
+ struct vmbus_channel_msginfo *info;
+ unsigned long flags;
+ int ret;
+
+ info = kzalloc(sizeof(struct vmbus_channel_msginfo) +
+ sizeof(struct vmbus_channel_modifychannel),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ init_completion(>waitevent);
+ info->waiting_channel = channel;
+
+ msg = (struct vmbus_channel_modifychannel *)info->msg;
+ msg->header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg->child_relid = channel->offermsg.child_relid;
+ msg->target_vp = target_vp;
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_add_tail(>msglistentry, _connection.chn_msg_list);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ ret = vmbus_post_msg(msg, sizeof(*msg), true);
+ trace_vmbus_send_modifychannel(msg, ret);
+ if (ret != 0) {
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock,
flags);
+ goto free_info;
+ }
+
+ /*
+* Release channel_mutex; otherwise, vmbus_onoffer_rescind() could
block on
+* the mutex and be unable to signal the completion.
+*
+* See the caller target_cpu_store() for information about the usage of
the
+* mutex.
+*/
+ mutex_unlock(_connection.channel_mutex);
+ wait_for_completion(>waitevent);
+ mutex_lock(_connection.channel_mutex);
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ if (info->response.modify_response.status)
+ ret = -EAGAIN;
+
+free_info:
+ kfree(info);
+ return ret;
+}
+
/*
* Set/change the vCPU (@target_vp) the channel (@child_relid) will interrupt.
*
- * CHANNELMSG_MODIFYCHANNEL messages are aynchronous. Also, Hyper-V does not
- * ACK such messages. IOW we can't know when the host will stop interrupting
- * the "old" vCPU and start interrupting the "new" vCPU for the given channel.
+ * CHANNELMSG_MODIFYCHANNEL messages are aynchronous. When VMbus version 5.3
+ * or later is negotiated, Hyper-V always sends an ACK in response to such a
+ * message. For VMbus version 5.2 and earlier, it never sends an ACK. With-
+ * out an ACK, we can not know when the host will stop interrupting the "old"
+ * vCPU and start interrupting the "new" vCPU for the given channel.
*
* The CHANNELMSG_MODIFYCHANNEL message type is supported since VMBus version
* VERSION_WIN10_V4_1.
*/
-int vmbus_send_modifychannel(u32 child_relid, u32 target_vp)
+int vmbus_send_modifychannel(struct vmbus_channel *channel, u32 target_vp)
{
- struct vmbus_channel_modifychannel conn_msg;
- int ret;
-
- memset(_msg, 0, sizeof(conn_msg));
- conn_msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
- conn_msg.child_relid = child_relid;
- conn_msg.target_vp = target_vp;
-
- ret = vmbus_post_msg(_msg, sizeof(conn_msg), true);
-
- trace_vmbus_send_modifychannel(_msg, ret);
-
- return ret;
+ if (vmbus_proto_version >= VERSION_WIN10_V5_3)
+ return send_modifychannel_with_ack(channel, target_vp);
+ return send_modifychannel_without_ack(channel, target_vp);
}
EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
diff --git a/drivers/hv/channel_mgmt.c b/dr
[PATCH v2 0/3] Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE
Changes since v1[1]: - rebase on hyperv-next - split changes into three patches - fix error handling in send_modifychannel_with_ack() - remove rescind checks in send_modifychannel_with_ack() - remove unneeded test in hv_synic_event_pending() - add/amend inline comments - style changes [1] https://lkml.kernel.org/r/20201126191210.13115-1-parri.and...@gmail.com Andrea Parri (Microsoft) (3): Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3 Drivers: hv: vmbus: Drivers: hv: vmbus: Introduce CHANNELMSG_MODIFYCHANNEL_RESPONSE Drivers: hv: vmbus: Check for pending channel interrupts before taking a CPU offline drivers/hv/channel.c | 99 --- drivers/hv/channel_mgmt.c | 42 + drivers/hv/connection.c | 3 +- drivers/hv/hv.c | 49 +++ drivers/hv/hv_trace.h | 15 ++ drivers/hv/vmbus_drv.c| 4 +- include/linux/hyperv.h| 13 - 7 files changed, 205 insertions(+), 20 deletions(-) -- 2.25.1
[PATCH v2 1/3] Drivers: hv: vmbus: Introduce and negotiate VMBus protocol version 5.3
Hyper-V has added VMBus protocol version 5.3. Allow Linux guests to
negotiate the new version on version of Hyper-V that support it.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/connection.c | 3 ++-
include/linux/hyperv.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 350e8c5cafa8c..dc19d5ae4373c 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -45,6 +45,7 @@ EXPORT_SYMBOL_GPL(vmbus_proto_version);
* Table of VMBus versions listed from newest to oldest.
*/
static __u32 vmbus_versions[] = {
+ VERSION_WIN10_V5_3,
VERSION_WIN10_V5_2,
VERSION_WIN10_V5_1,
VERSION_WIN10_V5,
@@ -60,7 +61,7 @@ static __u32 vmbus_versions[] = {
* Maximal VMBus protocol version guests can negotiate. Useful to cap the
* VMBus version for testing and debugging purpose.
*/
-static uint max_version = VERSION_WIN10_V5_2;
+static uint max_version = VERSION_WIN10_V5_3;
module_param(max_version, uint, S_IRUGO);
MODULE_PARM_DESC(max_version,
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 2c18c8e768efe..d6a6f76040b5f 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -234,6 +234,7 @@ static inline u32 hv_get_avail_to_write_percent(
* 5 . 0 (Newer Windows 10)
* 5 . 1 (Windows 10 RS4)
* 5 . 2 (Windows Server 2019, RS5)
+ * 5 . 3 (Windows Server 2021) // FIXME: use proper version number/name
*/
#define VERSION_WS2008 ((0 << 16) | (13))
@@ -245,6 +246,7 @@ static inline u32 hv_get_avail_to_write_percent(
#define VERSION_WIN10_V5 ((5 << 16) | (0))
#define VERSION_WIN10_V5_1 ((5 << 16) | (1))
#define VERSION_WIN10_V5_2 ((5 << 16) | (2))
+#define VERSION_WIN10_V5_3 ((5 << 16) | (3))
/* Make maximum size of pipe payload of 16K */
#define MAX_PIPE_DATA_PAYLOAD (sizeof(u8) * 16384)
--
2.25.1
Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote:
> The "open_info" variable is added to the _connection.chn_msg_list,
> but the error handling frees "open_info" without removing it from the
> list. This will result in a use after free. First remove it from the
> list, and then free it.
>
> Fixes: 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")
> Signed-off-by: Dan Carpenter
I had this 'queued' in my list,
Reviewed-by: Andrea Parri
Andrea
> ---
> From static analysis. Untested etc. There is almost certainly a good
> reason to add it to the list before checking "newchannel->rescind" but I
> don't know the code well enough to know what the reason is.
>
> drivers/hv/channel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index db30be8f9cce..1c5a418c1962 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -653,7 +653,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>
> if (newchannel->rescind) {
> err = -ENODEV;
> - goto error_free_info;
> + goto error_clean_msglist;
> }
>
> err = vmbus_post_msg(open_msg,
> --
> 2.30.2
>
Re: [RFC PATCH hyperv-next] scsi: storvsc: Use blk_mq_unique_tag() to generate requestIDs
On Fri, Apr 09, 2021 at 03:38:14PM +, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) Sent: Thursday, April
> 8, 2021 9:13 AM
> >
> > Use blk_mq_unique_tag() to generate requestIDs for StorVSC, avoiding
> > all issues with allocating enough entries in the VMbus requestor.
>
> This looks good to me! I'm glad to see that the idea worked without
> too much complexity.
>
> See a few comments inline below.
Thank you for these suggestions; I've tried to implement them, cf. the
diff at the bottom of this email (on top of this RFC, plus 'change the
storvsc callbacks to 'static''). I like the result, however this does
not work well yet: I am getting 'Incorrect transaction id' messages at
boot time with this diff; I'll dig more tomorrow... hints are welcome!
Andrea
>
> >
> > Suggested-by: Michael Kelley
> > Signed-off-by: Andrea Parri (Microsoft)
> > ---
> > drivers/hv/channel.c | 14 +++---
> > drivers/hv/ring_buffer.c | 12 ++---
> > drivers/net/hyperv/netvsc.c | 8 ++--
> > drivers/net/hyperv/rndis_filter.c | 2 +
> > drivers/scsi/storvsc_drv.c| 73 ++-
> > include/linux/hyperv.h| 13 +-
> > 6 files changed, 92 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index db30be8f9ccea..f78e02ace51e8 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -1121,15 +1121,14 @@ EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> > * vmbus_next_request_id - Returns a new request id. It is also
> > * the index at which the guest memory address is stored.
> > * Uses a spin lock to avoid race conditions.
> > - * @rqstor: Pointer to the requestor struct
> > + * @channel: Pointer to the VMbus channel struct
> > * @rqst_add: Guest memory address to be stored in the array
> > */
> > -u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> > +u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr)
> > {
> > + struct vmbus_requestor *rqstor = >requestor;
> > unsigned long flags;
> > u64 current_id;
> > - const struct vmbus_channel *channel =
> > - container_of(rqstor, const struct vmbus_channel, requestor);
> >
> > /* Check rqstor has been initialized */
> > if (!channel->rqstor_size)
> > @@ -1163,16 +1162,15 @@ EXPORT_SYMBOL_GPL(vmbus_next_request_id);
> > /*
> > * vmbus_request_addr - Returns the memory address stored at @trans_id
> > * in @rqstor. Uses a spin lock to avoid race conditions.
> > - * @rqstor: Pointer to the requestor struct
> > + * @channel: Pointer to the VMbus channel struct
> > * @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
> > * next request id.
> > */
> > -u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
> > +u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id)
> > {
> > + struct vmbus_requestor *rqstor = >requestor;
> > unsigned long flags;
> > u64 req_addr;
> > - const struct vmbus_channel *channel =
> > - container_of(rqstor, const struct vmbus_channel, requestor);
> >
> > /* Check rqstor has been initialized */
> > if (!channel->rqstor_size)
> > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> > index ecd82ebfd5bc4..46d8e038e4ee1 100644
> > --- a/drivers/hv/ring_buffer.c
> > +++ b/drivers/hv/ring_buffer.c
> > @@ -310,10 +310,12 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> > */
> >
> > if (desc->flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) {
> > - rqst_id = vmbus_next_request_id(>requestor, requestid);
> > - if (rqst_id == VMBUS_RQST_ERROR) {
> > - spin_unlock_irqrestore(_info->ring_lock, flags);
> > - return -EAGAIN;
> > + if (channel->next_request_id_callback != NULL) {
> > + rqst_id = channel->next_request_id_callback(channel,
> > requestid);
> > + if (rqst_id == VMBUS_RQST_ERROR) {
> > +
> > spin_unlock_irqrestore(_info->ring_lock, flags);
> > + return -EAGAIN;
> > + }
> > }
> > }
> > desc = hv_get_ring_buffer(outring_info) + old_write;
> > @@ -341,7 +343,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> > if (channel->rescind) {
> >
Re: [PATCH hyperv-next] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer
On Fri, Apr 09, 2021 at 03:49:00PM +, Michael Kelley wrote: > From: Andrea Parri (Microsoft) Sent: Thursday, April > 8, 2021 9:15 AM > > > > Pointers to ring-buffer packets sent by Hyper-V are used within the > > guest VM. Hyper-V can send packets with erroneous values or modify > > packet fields after they are processed by the guest. To defend > > against these scenarios, return a copy of the incoming VMBus packet > > after validating its length and offset fields in hv_pkt_iter_first(). > > In this way, the packet can no longer be modified by the host. > > Andrea -- has anything changed in this version of this patch, except > the value of NETVSC_MAX_XFER_PAGE_RANGES? It used to be a > fixed 375, but now is NVSP_RSC_MAX, which is 562. > > If that's the only change, then > > Reviewed-by: Michael Kelley The only change here is indeed the value of NETVSC_MAX_XFER_PAGE_RANGES, apologies for the omission of the changelog. Thanks for the review. Andrea
[PATCH hyperv-next] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer
From: Andres Beltran
Pointers to ring-buffer packets sent by Hyper-V are used within the
guest VM. Hyper-V can send packets with erroneous values or modify
packet fields after they are processed by the guest. To defend
against these scenarios, return a copy of the incoming VMBus packet
after validating its length and offset fields in hv_pkt_iter_first().
In this way, the packet can no longer be modified by the host.
Signed-off-by: Andres Beltran
Co-developed-by: Andrea Parri (Microsoft)
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel.c | 9 ++--
drivers/hv/hv_fcopy.c | 1 +
drivers/hv/hv_kvp.c | 1 +
drivers/hv/hyperv_vmbus.h | 2 +-
drivers/hv/ring_buffer.c | 82 ++-
drivers/net/hyperv/hyperv_net.h | 7 +++
drivers/net/hyperv/netvsc.c | 2 +
drivers/net/hyperv/rndis_filter.c | 2 +
drivers/scsi/storvsc_drv.c| 10
include/linux/hyperv.h| 48 +++---
net/vmw_vsock/hyperv_transport.c | 4 +-
11 files changed, 143 insertions(+), 25 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9ccea..b665db21e120d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -597,12 +597,15 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
newchannel->onchannel_callback = onchannelcallback;
newchannel->channel_callback_context = context;
- err = hv_ringbuffer_init(>outbound, page, send_pages);
+ if (!newchannel->max_pkt_size)
+ newchannel->max_pkt_size = VMBUS_DEFAULT_MAX_PKT_SIZE;
+
+ err = hv_ringbuffer_init(>outbound, page, send_pages, 0);
if (err)
goto error_clean_ring;
- err = hv_ringbuffer_init(>inbound,
-[send_pages], recv_pages);
+ err = hv_ringbuffer_init(>inbound, [send_pages],
+recv_pages, newchannel->max_pkt_size);
if (err)
goto error_clean_ring;
diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index 59ce85e00a028..660036da74495 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -349,6 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
{
recv_buffer = srv->recv_buffer;
fcopy_transaction.recv_channel = srv->channel;
+ fcopy_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
/*
* When this driver loads, the user level daemon that
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index b49962d312cef..c698592b83e42 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -757,6 +757,7 @@ hv_kvp_init(struct hv_util_service *srv)
{
recv_buffer = srv->recv_buffer;
kvp_transaction.recv_channel = srv->channel;
+ kvp_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 4;
/*
* When this driver loads, the user level daemon that
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 9416e09ebd58c..42f3d9d123a12 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -174,7 +174,7 @@ extern int hv_synic_cleanup(unsigned int cpu);
void hv_ringbuffer_pre_init(struct vmbus_channel *channel);
int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
- struct page *pages, u32 pagecnt);
+ struct page *pages, u32 pagecnt, u32 max_pkt_size);
void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index ecd82ebfd5bc4..848f3bba83f8b 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -190,7 +190,7 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)
/* Initialize the ring buffer. */
int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
- struct page *pages, u32 page_cnt)
+ struct page *pages, u32 page_cnt, u32 max_pkt_size)
{
int i;
struct page **pages_wraparound;
@@ -232,6 +232,14 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info
*ring_info,
sizeof(struct hv_ring_buffer);
ring_info->priv_read_index = 0;
+ /* Initialize buffer that holds copies of incoming packets */
+ if (max_pkt_size) {
+ ring_info->pkt_buffer = kzalloc(max_pkt_size, GFP_KERNEL);
+ if (!ring_info->pkt_buffer)
+ return -ENOMEM;
+ ring_info->pkt_buffer_size = max_pkt_size;
+ }
+
spin_lock_init(_info->ring_lock);
return 0;
@@ -244,6 +252,9 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info
*ring_info)
vunmap(ring_info->ring_buffer);
ring_info->ring_buffer = NULL;
mutex_unlock(_info->ring_buffer_mutex);
+
+ kfree(ring_info->pkt_buffer);
+ ring_in
[RFC PATCH hyperv-next] scsi: storvsc: Use blk_mq_unique_tag() to generate requestIDs
Use blk_mq_unique_tag() to generate requestIDs for StorVSC, avoiding
all issues with allocating enough entries in the VMbus requestor.
Suggested-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel.c | 14 +++---
drivers/hv/ring_buffer.c | 12 ++---
drivers/net/hyperv/netvsc.c | 8 ++--
drivers/net/hyperv/rndis_filter.c | 2 +
drivers/scsi/storvsc_drv.c| 73 ++-
include/linux/hyperv.h| 13 +-
6 files changed, 92 insertions(+), 30 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9ccea..f78e02ace51e8 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -1121,15 +1121,14 @@ EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
* vmbus_next_request_id - Returns a new request id. It is also
* the index at which the guest memory address is stored.
* Uses a spin lock to avoid race conditions.
- * @rqstor: Pointer to the requestor struct
+ * @channel: Pointer to the VMbus channel struct
* @rqst_add: Guest memory address to be stored in the array
*/
-u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
+u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr)
{
+ struct vmbus_requestor *rqstor = >requestor;
unsigned long flags;
u64 current_id;
- const struct vmbus_channel *channel =
- container_of(rqstor, const struct vmbus_channel, requestor);
/* Check rqstor has been initialized */
if (!channel->rqstor_size)
@@ -1163,16 +1162,15 @@ EXPORT_SYMBOL_GPL(vmbus_next_request_id);
/*
* vmbus_request_addr - Returns the memory address stored at @trans_id
* in @rqstor. Uses a spin lock to avoid race conditions.
- * @rqstor: Pointer to the requestor struct
+ * @channel: Pointer to the VMbus channel struct
* @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
* next request id.
*/
-u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
+u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id)
{
+ struct vmbus_requestor *rqstor = >requestor;
unsigned long flags;
u64 req_addr;
- const struct vmbus_channel *channel =
- container_of(rqstor, const struct vmbus_channel, requestor);
/* Check rqstor has been initialized */
if (!channel->rqstor_size)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index ecd82ebfd5bc4..46d8e038e4ee1 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -310,10 +310,12 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
*/
if (desc->flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) {
- rqst_id = vmbus_next_request_id(>requestor, requestid);
- if (rqst_id == VMBUS_RQST_ERROR) {
- spin_unlock_irqrestore(_info->ring_lock, flags);
- return -EAGAIN;
+ if (channel->next_request_id_callback != NULL) {
+ rqst_id = channel->next_request_id_callback(channel,
requestid);
+ if (rqst_id == VMBUS_RQST_ERROR) {
+
spin_unlock_irqrestore(_info->ring_lock, flags);
+ return -EAGAIN;
+ }
}
}
desc = hv_get_ring_buffer(outring_info) + old_write;
@@ -341,7 +343,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
if (channel->rescind) {
if (rqst_id != VMBUS_NO_RQSTOR) {
/* Reclaim request ID to avoid leak of IDs */
- vmbus_request_addr(>requestor, rqst_id);
+ channel->request_addr_callback(channel, rqst_id);
}
return -ENODEV;
}
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index c64cc7639c39c..1a221ce2d6fdc 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -730,7 +730,7 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
int queue_sends;
u64 cmd_rqst;
- cmd_rqst = vmbus_request_addr(>requestor, (u64)desc->trans_id);
+ cmd_rqst = channel->request_addr_callback(channel, (u64)desc->trans_id);
if (cmd_rqst == VMBUS_RQST_ERROR) {
netdev_err(ndev, "Incorrect transaction id\n");
return;
@@ -790,8 +790,8 @@ static void netvsc_send_completion(struct net_device *ndev,
/* First check if this is a VMBUS completion without data payload */
if (!msglen) {
- cmd_rqst = vmbus_request_addr(_channel->requestor,
- (u64)desc->trans_id);
+ cmd_rqst =
incoming_channel->request_addr_callback(incoming_channel,
+
Re: [PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()
Hi Olaf,
On Mon, Mar 29, 2021 at 06:37:21PM +0200, Olaf Hering wrote:
> On Thu, Dec 17, Andrea Parri (Microsoft) wrote:
>
> > Check that the packet is of the expected size at least, don't copy data
> > past the packet.
>
> > + if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
> > + stor_device->vmscsi_size_delta) {
> > + dev_err(>device, "Invalid packet len\n");
> > + continue;
> > + }
> > +
>
> Sorry for being late:
>
> It might be just cosmetic, but should this check be done prior the call to
> vmbus_request_addr()?
TBH, I'm not immediately seeing why it 'should'; it could make sense to move
the check on the packet data length.
> Unrelated: my copy of vmbus_request_addr() can return 0, which is apparently
> not handled by this loop in storvsc_on_channel_callback().
Indeed, IDs of 0 are reserved for so called unsolicited messages; I think we
should check that storvsc_on_io_completion() is not called on such messages.
Thanks,
Andrea
[PATCH] Drivers: hv: vmbus: Drop error message when 'No request id available'
Running out of request IDs on a channel essentially produces the same
effect as running out of space in the ring buffer, in that -EAGAIN is
returned. The error message in hv_ringbuffer_write() should either be
dropped (since we don't output a message when the ring buffer is full)
or be made conditional/debug-only.
Suggested-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
Fixes: e8b7db38449ac ("Drivers: hv: vmbus: Add vmbus_requestor data structure
for VMBus hardening")
---
drivers/hv/ring_buffer.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 35833d4d1a1dc..ecd82ebfd5bc4 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -313,7 +313,6 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
rqst_id = vmbus_next_request_id(>requestor, requestid);
if (rqst_id == VMBUS_RQST_ERROR) {
spin_unlock_irqrestore(_info->ring_lock, flags);
- pr_err("No request id available\n");
return -EAGAIN;
}
}
--
2.25.1
[PATCH net] hv_netvsc: Fix validation in netvsc_linkstatus_callback()
Contrary to the RNDIS protocol specification, certain (pre-Fe)
implementations of Hyper-V's vSwitch did not account for the status
buffer field in the length of an RNDIS packet; the bug was fixed in
newer implementations. Validate the status buffer fields using the
length of the 'vmtransfer_page' packet (all implementations), that
is known/validated to be less than or equal to the receive section
size and not smaller than the length of the RNDIS message.
Reported-by: Dexuan Cui
Suggested-by: Haiyang Zhang
Signed-off-by: Andrea Parri (Microsoft)
Fixes: 505e3f00c3f36 ("hv_netvsc: Add (more) validation for untrusted Hyper-V
values")
---
drivers/net/hyperv/hyperv_net.h | 2 +-
drivers/net/hyperv/netvsc_drv.c | 13 +
drivers/net/hyperv/rndis_filter.c | 2 +-
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index e1a497d3c9ba4..59ac04a610adb 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -229,7 +229,7 @@ int netvsc_send(struct net_device *net,
bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
struct rndis_message *resp,
- void *data);
+ void *data, u32 data_buflen);
int netvsc_recv_callback(struct net_device *net,
struct netvsc_device *nvdev,
struct netvsc_channel *nvchan);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 8176fa0c8b168..15f262b70489e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -744,7 +744,7 @@ static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
*/
void netvsc_linkstatus_callback(struct net_device *net,
struct rndis_message *resp,
- void *data)
+ void *data, u32 data_buflen)
{
struct rndis_indicate_status *indicate = >msg.indicate_status;
struct net_device_context *ndev_ctx = netdev_priv(net);
@@ -765,11 +765,16 @@ void netvsc_linkstatus_callback(struct net_device *net,
if (indicate->status == RNDIS_STATUS_LINK_SPEED_CHANGE) {
u32 speed;
- /* Validate status_buf_offset */
+ /* Validate status_buf_offset and status_buflen.
+*
+* Certain (pre-Fe) implementations of Hyper-V's vSwitch didn't
account
+* for the status buffer field in resp->msg_len; perform the
validation
+* using data_buflen (>= resp->msg_len).
+*/
if (indicate->status_buflen < sizeof(speed) ||
indicate->status_buf_offset < sizeof(*indicate) ||
- resp->msg_len - RNDIS_HEADER_SIZE <
indicate->status_buf_offset ||
- resp->msg_len - RNDIS_HEADER_SIZE -
indicate->status_buf_offset
+ data_buflen - RNDIS_HEADER_SIZE <
indicate->status_buf_offset ||
+ data_buflen - RNDIS_HEADER_SIZE -
indicate->status_buf_offset
< indicate->status_buflen) {
netdev_err(net, "invalid rndis_indicate_status
packet\n");
return;
diff --git a/drivers/net/hyperv/rndis_filter.c
b/drivers/net/hyperv/rndis_filter.c
index 123cc9d25f5ed..c0e89e107d575 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -620,7 +620,7 @@ int rndis_filter_receive(struct net_device *ndev,
case RNDIS_MSG_INDICATE:
/* notification msgs */
- netvsc_linkstatus_callback(ndev, rndis_msg, data);
+ netvsc_linkstatus_callback(ndev, rndis_msg, data, buflen);
break;
default:
netdev_err(ndev,
--
2.25.1
Re: [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host
On Wed, Feb 24, 2021 at 02:16:00PM +0100, Andrea Parri wrote: > On Wed, Feb 24, 2021 at 07:50:08AM -0500, Sasha Levin wrote: > > From: "Andrea Parri (Microsoft)" > > > > [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ] > > > > __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 > > Signed-off-by: Andrea Parri (Microsoft) > > Reviewed-by: Michael Kelley > > Link: > > https://lore.kernel.org/r/20201209070827.29335-2-parri.and...@gmail.com > > Signed-off-by: Wei Liu > > Signed-off-by: Sasha Levin > > Sasha - This patch is one of a group of patches where a Linux guest running on > Hyper-V will start assuming that hypervisor behavior might be malicious, and > guards against such behavior. Because this is a new assumption, these patches > are more properly treated as new functionality rather than as bug fixes. So I > would propose that we *not* bring such patches back to stable branches. For future/similar cases: I'm wondering, is there some way to annotate a patch with "please do not bring it back"? Thanks, Andrea
Re: [PATCH AUTOSEL 4.14 15/16] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
On Wed, Feb 24, 2021 at 07:55:12AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ]
>
> 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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Same here.
Andrea
> ---
> 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 5bf633c15cd4b..6ddda97030628 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -942,6 +942,18 @@ static void vmbus_onoffer_rescind(struct
> vmbus_channel_message_header *hdr)
>
> mutex_lock(_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
> + * _connection.channel_mutex.
> + */
> + if (channel->rescind_ref) {
> + mutex_unlock(_connection.channel_mutex);
> + return;
> + }
> + channel->rescind_ref = true;
> + }
> mutex_unlock(_connection.channel_mutex);
>
> if (channel == NULL) {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 63cd81e5610d1..22e2c2d75361e 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -710,6 +710,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.27.0
>
Re: [PATCH AUTOSEL 5.10 40/56] Drivers: hv: vmbus: Initialize memory to be sent to the host
On Wed, Feb 24, 2021 at 07:51:56AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ]
>
> __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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Same here.
Andrea
> ---
> 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 fbdda9938039a..f9f04b5cd303f 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -548,7 +548,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) {
> @@ -674,7 +674,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.27.0
>
Re: [PATCH AUTOSEL 5.10 41/56] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
On Wed, Feb 24, 2021 at 07:51:57AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ]
>
> 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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Same here.
Andrea
> ---
> 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 1d44bb635bb84..a9f58840f85dc 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -1049,6 +1049,18 @@ static void vmbus_onoffer_rescind(struct
> vmbus_channel_message_header *hdr)
>
> mutex_lock(_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
> + * _connection.channel_mutex.
> + */
> + if (channel->rescind_ref) {
> + mutex_unlock(_connection.channel_mutex);
> + return;
> + }
> + channel->rescind_ref = true;
> + }
> mutex_unlock(_connection.channel_mutex);
>
> if (channel == NULL) {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 1ce131f29f3b4..376f0f9e19650 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -786,6 +786,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.27.0
>
Re: [PATCH AUTOSEL 5.4 30/40] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
On Wed, Feb 24, 2021 at 07:53:30AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ]
>
> 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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Same here.
Andrea
> ---
> 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 452307c79e4b9..dd4e890cf1b1d 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -1048,6 +1048,18 @@ static void vmbus_onoffer_rescind(struct
> vmbus_channel_message_header *hdr)
>
> mutex_lock(_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
> + * _connection.channel_mutex.
> + */
> + if (channel->rescind_ref) {
> + mutex_unlock(_connection.channel_mutex);
> + return;
> + }
> + channel->rescind_ref = true;
> + }
> mutex_unlock(_connection.channel_mutex);
>
> if (channel == NULL) {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 67d9b5a374600..51e2134b32a21 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -734,6 +734,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.27.0
>
Re: [PATCH AUTOSEL 4.19 21/26] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
On Wed, Feb 24, 2021 at 07:54:29AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ]
>
> 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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Same here.
Andrea
> ---
> 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 7920b0d7e35a7..1322e799938af 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -954,6 +954,18 @@ static void vmbus_onoffer_rescind(struct
> vmbus_channel_message_header *hdr)
>
> mutex_lock(_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
> + * _connection.channel_mutex.
> + */
> + if (channel->rescind_ref) {
> + mutex_unlock(_connection.channel_mutex);
> + return;
> + }
> + channel->rescind_ref = true;
> + }
> mutex_unlock(_connection.channel_mutex);
>
> if (channel == NULL) {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 35461d49d3aee..59525fe25abde 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -736,6 +736,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.27.0
>
Re: [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
On Wed, Feb 24, 2021 at 07:50:09AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ]
>
> 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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Sasha - This patch is one of a group of patches where a Linux guest running on
Hyper-V will start assuming that hypervisor behavior might be malicious, and
guards against such behavior. Because this is a new assumption, these patches
are more properly treated as new functionality rather than as bug fixes. So I
would propose that we *not* bring such patches back to stable branches.
Thanks,
Andrea
> ---
> 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 1d44bb635bb84..a9f58840f85dc 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -1049,6 +1049,18 @@ static void vmbus_onoffer_rescind(struct
> vmbus_channel_message_header *hdr)
>
> mutex_lock(_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
> + * _connection.channel_mutex.
> + */
> + if (channel->rescind_ref) {
> + mutex_unlock(_connection.channel_mutex);
> + return;
> + }
> + channel->rescind_ref = true;
> + }
> mutex_unlock(_connection.channel_mutex);
>
> if (channel == NULL) {
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 5ddb479c4d4cb..ef3573e99d989 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -803,6 +803,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.27.0
>
Re: [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host
On Wed, Feb 24, 2021 at 07:50:08AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ]
>
> __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
> Signed-off-by: Andrea Parri (Microsoft)
> Reviewed-by: Michael Kelley
> Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.and...@gmail.com
> Signed-off-by: Wei Liu
> Signed-off-by: Sasha Levin
Sasha - This patch is one of a group of patches where a Linux guest running on
Hyper-V will start assuming that hypervisor behavior might be malicious, and
guards against such behavior. Because this is a new assumption, these patches
are more properly treated as new functionality rather than as bug fixes. So I
would propose that we *not* bring such patches back to stable branches.
Thanks,
Andrea
> ---
> 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 6fb0c76bfbf81..0bd202de79600 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -618,7 +618,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) {
> @@ -745,7 +745,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.27.0
>
Regressions with VMBus/VSCs hardening changes
Hi all,
I'm reporting two regressions following certain VMBus/VSCs hardening changes
we've been discussing 'recently', unfortunately the first regression already
touched/affects mainline while the second one is in hyperv-next:
1) [mainline]
The first regression manifests with the following message (several):
hv_vmbus: No request id available
I could reliably reproduce such message/behavior by running the command:
fio --name=seqwrite --rw=read --direct=1 --ioengine=libaio --bs=32k
--numjobs=4 --size=2G --runtime=60
(the message is triggered when files are being created).
I've bisected this regression to commit:
453de21c2b8281 ("scsi: storvsc: Use vmbus_requestor to generate transaction
IDs for VMBus hardening")
2) [hyperv-next]
The second regression manifests with various messages including:
hv_netvsc 9c5f5000-0499-4b18-b2eb-a8d5c57c8774 eth0: Unknown nvsp packet type
received 51966
hv_netvsc 9c5f5000-0499-4b18-b2eb-a8d5c57c8774 eth0: unhandled packet type 0,
tid 0
hv_netvsc 9c5f5000-0499-4b18-b2eb-a8d5c57c8774 eth0: Incorrect transaction id
hv_netvsc 9c5f5000-0499-4b18-b2eb-a8d5c57c8774 eth0: Invalid rndis_msg
(buflen: 262, msg_len: 1728)
The connection was then typically lost/reset by the peer.
I could reproduce such behavior/messages by running the test:
ntttcp -r -m 8,*, # receiver
ntttcp -s -m 8,*, -ns -t 60 # sender
I bisected this regression to commit:
a8c3209998afb5 ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the
ring buffer")
---
I am investigating but don't have fixes for these regressions now: given the
'timing' (-rc7 with the next merge window at the door...) I would propose to
revert/drop the interested changes:
1) 453de21c2b8281 is part of the so called 'vmbus_requestor' series that was
applied during the merge window for 5.11:
e8b7db38449ac5 ("Drivers: hv: vmbus: Add vmbus_requestor data structure for
VMBus hardening")
453de21c2b8281 ("scsi: storvsc: Use vmbus_requestor to generate transaction
IDs for VMBus hardening")
4d18fcc95f5095 ("hv_netvsc: Use vmbus_requestor to generate transaction IDs
for VMBus hardening")
I could prepare/submit patches to revert such commits (asap but likely not
before tomorrow/late Saturday - EU time).
2) IIUC a8c3209998afb5 could be dropped (after rebase) without further modi-
fications to hyperv-next.
Other suggestions/thoughts?
Thanks,
Andrea
[PATCH net-next 2/2] hv_netvsc: Load and store the proper (NBL_HASH_INFO) per-packet info
Fix the typo.
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
Fixes: 0ba35fe91ce34f ("hv_netvsc: Copy packets sent by Hyper-V out of the
receive buffer")
---
drivers/net/hyperv/rndis_filter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/rndis_filter.c
b/drivers/net/hyperv/rndis_filter.c
index 6c48a4d627368..0c2ebe7ac6554 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -465,7 +465,7 @@ void rsc_add_data(struct netvsc_channel *nvchan,
}
nvchan->rsc.pktlen = len;
if (hash_info != NULL) {
- nvchan->rsc.csum_info = *csum_info;
+ nvchan->rsc.hash_info = *hash_info;
nvchan->rsc.ppi_flags |= NVSC_RSC_HASH_INFO;
} else {
nvchan->rsc.ppi_flags &= ~NVSC_RSC_HASH_INFO;
--
2.25.1
[PATCH net] hv_netvsc: Reset the RSC count if NVSP_STAT_FAIL in netvsc_receive()
Commit 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V
values") added validation to rndis_filter_receive_data() (and
rndis_filter_receive()) which introduced NVSP_STAT_FAIL-scenarios where
the count is not updated/reset. Fix this omission, and prevent similar
scenarios from occurring in the future.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
Fixes: 44144185951a0f ("hv_netvsc: Add validation for untrusted Hyper-V values")
---
drivers/net/hyperv/netvsc.c | 5 -
drivers/net/hyperv/rndis_filter.c | 2 --
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 2350342b961ff..13bd48a75db76 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1262,8 +1262,11 @@ static int netvsc_receive(struct net_device *ndev,
ret = rndis_filter_receive(ndev, net_device,
nvchan, data, buflen);
- if (unlikely(ret != NVSP_STAT_SUCCESS))
+ if (unlikely(ret != NVSP_STAT_SUCCESS)) {
+ /* Drop incomplete packet */
+ nvchan->rsc.cnt = 0;
status = NVSP_STAT_FAIL;
+ }
}
enq_receive_complete(ndev, net_device, q_idx,
diff --git a/drivers/net/hyperv/rndis_filter.c
b/drivers/net/hyperv/rndis_filter.c
index 598713c0d5a87..3aab2b867fc0d 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -509,8 +509,6 @@ static int rndis_filter_receive_data(struct net_device
*ndev,
return ret;
drop:
- /* Drop incomplete packet */
- nvchan->rsc.cnt = 0;
return NVSP_STAT_FAIL;
}
--
2.25.1
[PATCH net-next 1/2] hv_netvsc: Allocate the recv_buf buffers after NVSP_MSG1_TYPE_SEND_RECV_BUF
The recv_buf buffers are allocated in netvsc_device_add(). Later in
netvsc_init_buf() the response to NVSP_MSG1_TYPE_SEND_RECV_BUF allows
the host to set up a recv_section_size that could be bigger than the
(default) value used for that allocation. The host-controlled value
could be used by a malicious host to bypass the check on the packet's
length in netvsc_receive() and hence to overflow the recv_buf buffer.
Move the allocation of the recv_buf buffers into netvsc_init_but().
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
Fixes: 0ba35fe91ce34f ("hv_netvsc: Copy packets sent by Hyper-V out of the
receive buffer")
---
drivers/net/hyperv/netvsc.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 0fba8257fc119..9db1ea3affbb3 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -311,7 +311,7 @@ static int netvsc_init_buf(struct hv_device *device,
struct nvsp_message *init_packet;
unsigned int buf_size;
size_t map_words;
- int ret = 0;
+ int i, ret = 0;
/* Get receive buffer area. */
buf_size = device_info->recv_sections * device_info->recv_section_size;
@@ -405,6 +405,16 @@ static int netvsc_init_buf(struct hv_device *device,
goto cleanup;
}
+ for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
+ struct netvsc_channel *nvchan = _device->chan_table[i];
+
+ nvchan->recv_buf = kzalloc(net_device->recv_section_size,
GFP_KERNEL);
+ if (nvchan->recv_buf == NULL) {
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+ }
+
/* Setup receive completion ring.
* Add 1 to the recv_section_cnt because at least one entry in a
* ring buffer has to be empty.
@@ -1549,12 +1559,6 @@ struct netvsc_device *netvsc_device_add(struct hv_device
*device,
for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
struct netvsc_channel *nvchan = _device->chan_table[i];
- nvchan->recv_buf = kzalloc(device_info->recv_section_size,
GFP_KERNEL);
- if (nvchan->recv_buf == NULL) {
- ret = -ENOMEM;
- goto cleanup2;
- }
-
nvchan->channel = device->channel;
nvchan->net_device = net_device;
u64_stats_init(>tx_stats.syncp);
--
2.25.1
[PATCH net-next 0/2] Amend "hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer"
Patch #2 also addresses the Smatch complaint reported here: https://lkml.kernel.org/r/YBp2oVIdMe+G%2FliJ@mwanda/ Thanks, Andrea Cc: "David S. Miller" Cc: Jakub Kicinski Cc: net...@vger.kernel.org Andrea Parri (Microsoft) (2): hv_netvsc: Allocate the recv_buf buffers after NVSP_MSG1_TYPE_SEND_RECV_BUF hv_netvsc: Load and store the proper (NBL_HASH_INFO) per-packet info drivers/net/hyperv/netvsc.c | 18 +++--- drivers/net/hyperv/rndis_filter.c | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) -- 2.25.1
Re: [PATCH v2 net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer
On Tue, Feb 02, 2021 at 11:45:49AM -0800, Jakub Kicinski wrote: > On Tue, 2 Feb 2021 09:18:43 +0100 Andrea Parri wrote: > > Hi net maintainers, > > > > > > On Sat, Jan 30, 2021 at 12:50:06AM +, > > patchwork-bot+netdev...@kernel.org wrote: > > > Hello: > > > > > > This patch was applied to netdev/net-next.git (refs/heads/master): > > > > > > On Tue, 26 Jan 2021 17:29:07 +0100 you wrote: > > > > Pointers to receive-buffer packets sent by Hyper-V are used within the > > > > guest VM. Hyper-V can send packets with erroneous values or modify > > > > packet fields after they are processed by the guest. To defend against > > > > these scenarios, copy (sections of) the incoming packet after validating > > > > their length and offset fields in netvsc_filter_receive(). In this way, > > > > the packet can no longer be modified by the host. > > > > > > > > [...] > > > > > > Here is the summary with links: > > > - [v2,net-next] hv_netvsc: Copy packets sent by Hyper-V out of the > > > receive buffer > > > https://git.kernel.org/netdev/net-next/c/0ba35fe91ce3 > > > > I'd have some fixes on top of this and I'm wondering about the process: > > would > > you consider fixes/patches on top of this commit now? > > Fixes for bugs present in Linus's tree? > > You need to target the net tree, and give us instructions on how to > resolve the conflict which will arise from merging net into net-next. > > > would you rather prefer me to squash these fixes into a v3? other? > > Networking trees are immutable, and v2 was already applied. We could > do a revert, apply fix, apply v3, but we prefer to just handle the > merge conflict. Thanks for the clarification, Jakub. And sorry for the confusion; let me just send out the 'fixes'/patches (I have one targeting the net tree and two targeting the net-next tree, with no conflict between them), so that they can be reviewed and we can agree /discuss any further steps. Thanks, Andrea
Re: [PATCH v2 net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer
Hi net maintainers, On Sat, Jan 30, 2021 at 12:50:06AM +, patchwork-bot+netdev...@kernel.org wrote: > Hello: > > This patch was applied to netdev/net-next.git (refs/heads/master): > > On Tue, 26 Jan 2021 17:29:07 +0100 you wrote: > > Pointers to receive-buffer packets sent by Hyper-V are used within the > > guest VM. Hyper-V can send packets with erroneous values or modify > > packet fields after they are processed by the guest. To defend against > > these scenarios, copy (sections of) the incoming packet after validating > > their length and offset fields in netvsc_filter_receive(). In this way, > > the packet can no longer be modified by the host. > > > > [...] > > Here is the summary with links: > - [v2,net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive > buffer > https://git.kernel.org/netdev/net-next/c/0ba35fe91ce3 I'd have some fixes on top of this and I'm wondering about the process: would you consider fixes/patches on top of this commit now? would you rather prefer me to squash these fixes into a v3? other? Thanks, Andrea
[PATCH v3 hyperv-next 2/4] Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests
Only the VSCs or ICs that have been hardened and that are critical for
the successful adoption of Confidential VMs should be allowed if the
guest is running isolated. This change reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel_mgmt.c | 38 ++
include/linux/hyperv.h| 1 +
2 files changed, 39 insertions(+)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..f0ed730e2e4e4 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -31,101 +31,118 @@ const struct vmbus_device vmbus_devs[] = {
{ .dev_type = HV_IDE,
HV_IDE_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* SCSI */
{ .dev_type = HV_SCSI,
HV_SCSI_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Fibre Channel */
{ .dev_type = HV_FC,
HV_SYNTHFC_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* Synthetic NIC */
{ .dev_type = HV_NIC,
HV_NIC_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Network Direct */
{ .dev_type = HV_ND,
HV_ND_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* PCIE */
{ .dev_type = HV_PCIE,
HV_PCIE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Frame Buffer */
{ .dev_type = HV_FB,
HV_SYNTHVID_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Keyboard */
{ .dev_type = HV_KBD,
HV_KBD_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic MOUSE */
{ .dev_type = HV_MOUSE,
HV_MOUSE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* KVP */
{ .dev_type = HV_KVP,
HV_KVP_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Time Synch */
{ .dev_type = HV_TS,
HV_TS_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Heartbeat */
{ .dev_type = HV_HB,
HV_HEART_BEAT_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Shutdown */
{ .dev_type = HV_SHUTDOWN,
HV_SHUTDOWN_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* File copy */
{ .dev_type = HV_FCOPY,
HV_FCOPY_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Backup */
{ .dev_type = HV_BACKUP,
HV_VSS_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Dynamic Memory */
{ .dev_type = HV_DM,
HV_DM_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Unknown GUID */
{ .dev_type = HV_UNKNOWN,
.perf_device = false,
+ .allowed_in_isolated = false,
},
};
@@ -903,6 +920,20 @@ find_primary_channel_by_offer(const struct
vmbus_channel_offer_channel *offer)
return channel;
}
+static bool vmbus_is_valid_device(const guid_t *guid)
+{
+ u16 i;
+
+ if (!hv_is_isolation_supported())
+ return true;
+
+ for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
+ if (guid_equal(guid, _devs[i].guid))
+ return vmbus_devs[i].allowed_in_isolated;
+ }
+ return false;
+}
+
/*
* vmbus_onoffer - Handler for channel offers from vmbus in parent partition.
*
@@ -917,6 +948,13 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
trace_vmbus_onoffer(offer);
+ if (!vmbus_is_valid_device(>offer.if_type)) {
+ pr_err_ratelimited("Invalid offer %d from the host supporting
isolation\n",
+ offer->child_relid);
+ atomic_dec(_connection.offer_in_progress);
+ return;
+ }
+
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index f0d48a368f131..e3426f8c12db9 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -789,6 +789,7 @@ struct vmbus_device {
u16 dev_type;
guid_t guid;
bool perf_device;
+ bool allowed_in_isolated;
};
#define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
--
2.25.1
[PATCH v3 hyperv-next 1/4] x86/hyperv: Load/save the Isolation Configuration leaf
If bit 22 of Group B Features is set, the guest has access to the
Isolation Configuration CPUID leaf. On x86, the first four bits
of EAX in this leaf provide the isolation type of the partition;
we entail three isolation types: 'SNP' (hardware-based isolation),
'VBS' (software-based isolation), and 'NONE' (no isolation).
Signed-off-by: Andrea Parri (Microsoft)
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: Arnd Bergmann
Cc: x...@kernel.org
Cc: linux-a...@vger.kernel.org
---
arch/x86/hyperv/hv_init.c | 15 +++
arch/x86/include/asm/hyperv-tlfs.h | 15 +++
arch/x86/kernel/cpu/mshyperv.c | 9 +
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 5 +
5 files changed, 45 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index e04d90af4c27c..ccdfc6868cfc8 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -528,3 +529,17 @@ bool hv_is_hibernation_supported(void)
return acpi_sleep_state_supported(ACPI_STATE_S4);
}
EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
+
+enum hv_isolation_type hv_get_isolation_type(void)
+{
+ if (!(ms_hyperv.features_b & HV_ISOLATION))
+ return HV_ISOLATION_TYPE_NONE;
+ return FIELD_GET(HV_ISOLATION_TYPE, ms_hyperv.isolation_config_b);
+}
+EXPORT_SYMBOL_GPL(hv_get_isolation_type);
+
+bool hv_is_isolation_supported(void)
+{
+ return hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;
+}
+EXPORT_SYMBOL_GPL(hv_is_isolation_supported);
diff --git a/arch/x86/include/asm/hyperv-tlfs.h
b/arch/x86/include/asm/hyperv-tlfs.h
index 6bf42aed387e3..6aed936e5e962 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -22,6 +22,7 @@
#define HYPERV_CPUID_ENLIGHTMENT_INFO 0x4004
#define HYPERV_CPUID_IMPLEMENT_LIMITS 0x4005
#define HYPERV_CPUID_NESTED_FEATURES 0x400A
+#define HYPERV_CPUID_ISOLATION_CONFIG 0x400C
#define HYPERV_CPUID_VIRT_STACK_INTERFACE 0x4081
#define HYPERV_VS_INTERFACE_EAX_SIGNATURE 0x31235356 /* "VS#1" */
@@ -122,6 +123,20 @@
#define HV_X64_NESTED_GUEST_MAPPING_FLUSH BIT(18)
#define HV_X64_NESTED_MSR_BITMAP BIT(19)
+/* HYPERV_CPUID_ISOLATION_CONFIG.EAX bits. */
+#define HV_PARAVISOR_PRESENT BIT(0)
+
+/* HYPERV_CPUID_ISOLATION_CONFIG.EBX bits. */
+#define HV_ISOLATION_TYPE GENMASK(3, 0)
+#define HV_SHARED_GPA_BOUNDARY_ACTIVE BIT(5)
+#define HV_SHARED_GPA_BOUNDARY_BITSGENMASK(11, 6)
+
+enum hv_isolation_type {
+ HV_ISOLATION_TYPE_NONE = 0,
+ HV_ISOLATION_TYPE_VBS = 1,
+ HV_ISOLATION_TYPE_SNP = 2
+};
+
/* Hyper-V specific model specific registers (MSRs) */
/* MSR used to identify the guest OS. */
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index f628e3dc150f3..ea7bd8dff171c 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -225,6 +225,7 @@ static void __init ms_hyperv_init_platform(void)
* Extract the features and hints
*/
ms_hyperv.features = cpuid_eax(HYPERV_CPUID_FEATURES);
+ ms_hyperv.features_b = cpuid_ebx(HYPERV_CPUID_FEATURES);
ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
ms_hyperv.hints= cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
@@ -259,6 +260,14 @@ static void __init ms_hyperv_init_platform(void)
x86_platform.calibrate_cpu = hv_get_tsc_khz;
}
+ if (ms_hyperv.features_b & HV_ISOLATION) {
+ ms_hyperv.isolation_config_a =
cpuid_eax(HYPERV_CPUID_ISOLATION_CONFIG);
+ ms_hyperv.isolation_config_b =
cpuid_ebx(HYPERV_CPUID_ISOLATION_CONFIG);
+
+ pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B
0x%x\n",
+ ms_hyperv.isolation_config_a,
ms_hyperv.isolation_config_b);
+ }
+
if (ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED) {
ms_hyperv.nested_features =
cpuid_eax(HYPERV_CPUID_NESTED_FEATURES);
diff --git a/include/asm-generic/hyperv-tlfs.h
b/include/asm-generic/hyperv-tlfs.h
index e73a11850055c..20d3cd9502043 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -89,6 +89,7 @@
#define HV_ACCESS_STATSBIT(8)
#define HV_DEBUGGING BIT(11)
#define HV_CPU_POWER_MANAGEMENTBIT(12)
+#define HV_ISOLATION BIT(22)
/*
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index c57799684170c..dff58a3db5d5c 100644
[PATCH v3 hyperv-next 3/4] Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests
Restrict the protocol version(s) that will be negotiated with the host
to be 5.2 or greater if the guest is running isolated. This reduces the
footprint of the code that will be exercised by Confidential VMs and
hence the exposure to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/connection.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 11170d9a2e1a5..c83612cddb995 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -244,6 +244,13 @@ int vmbus_connect(void)
break;
}
+ if (hv_is_isolation_supported() && version < VERSION_WIN10_V5_2) {
+ pr_err("Invalid VMBus version %d.%d (expected >= %d.%d) from
the host supporting isolation\n",
+ version >> 16, version & 0x, VERSION_WIN10_V5_2 >>
16, VERSION_WIN10_V5_2 & 0x);
+ ret = -EINVAL;
+ goto cleanup;
+ }
+
vmbus_proto_version = version;
pr_info("Vmbus version:%d.%d\n",
version >> 16, version & 0x);
--
2.25.1
[PATCH v3 hyperv-next 4/4] hv_netvsc: Restrict configurations on isolated guests
Restrict the NVSP protocol version(s) that will be negotiated with the
host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
isolated. Moreover, do not advertise the SR-IOV capability and ignore
NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests, which
are not supposed to support SR-IOV. This reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
Acked-by: Jakub Kicinski
Reviewed-by: Haiyang Zhang
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
drivers/net/hyperv/netvsc.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..51005f2d4a821 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -22,6 +22,7 @@
#include
#include
+#include
#include "hyperv_net.h"
#include "netvsc_trace.h"
@@ -544,7 +545,10 @@ static int negotiate_nvsp_ver(struct hv_device *device,
init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
- init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
+ if (hv_is_isolation_supported())
+ netdev_info(ndev, "SR-IOV not advertised by guests on
the host supporting isolation\n");
+ else
+
init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
/* Teaming bit is needed to receive link speed updates */
init_packet->msg.v2_msg.send_ndis_config.capability.teaming = 1;
@@ -591,6 +595,13 @@ static int netvsc_connect_vsp(struct hv_device *device,
goto cleanup;
}
+ if (hv_is_isolation_supported() && net_device->nvsp_version <
NVSP_PROTOCOL_VERSION_61) {
+ netdev_err(ndev, "Invalid NVSP version 0x%x (expected >= 0x%x)
from the host supporting isolation\n",
+ net_device->nvsp_version, NVSP_PROTOCOL_VERSION_61);
+ ret = -EPROTO;
+ goto cleanup;
+ }
+
pr_debug("Negotiated NVSP version:%x\n", net_device->nvsp_version);
/* Send the ndis version */
@@ -1357,7 +1368,10 @@ static void netvsc_receive_inband(struct net_device
*ndev,
break;
case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
- netvsc_send_vf(ndev, nvmsg, msglen);
+ if (hv_is_isolation_supported())
+ netdev_err(ndev, "Ignore VF_ASSOCIATION msg from the
host supporting isolation\n");
+ else
+ netvsc_send_vf(ndev, nvmsg, msglen);
break;
}
}
--
2.25.1
[PATCH v3 hyperv-next 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests
Changes since v2 [1]: - improve/add logging (Michael Kelley) - rename 'hypercalls_features' to 'features_b' (Michael Kelley) - move VMBus and NVSC version checks after 'for' loop (Michael Kelley) - remove/inline helper functions (Michael Kelley) - other minor changes (Michael Kelley) Changes since v1 [2]: - improve/add logging (Haiyang Zhang) - move NVSC version check after version negotiation (Haiyang Zhang) [1] https://lkml.kernel.org/r/20210126115641.2527-1-parri.and...@gmail.com [1] https://lkml.kernel.org/r/20210119175841.22248-1-parri.and...@gmail.com Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: "H. Peter Anvin" Cc: Arnd Bergmann Cc: "David S. Miller" Cc: Jakub Kicinski Cc: x...@kernel.org Cc: linux-a...@vger.kernel.org Cc: net...@vger.kernel.org Andrea Parri (Microsoft) (4): x86/hyperv: Load/save the Isolation Configuration leaf Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests hv_netvsc: Restrict configurations on isolated guests arch/x86/hyperv/hv_init.c | 15 arch/x86/include/asm/hyperv-tlfs.h | 15 arch/x86/kernel/cpu/mshyperv.c | 9 +++ drivers/hv/channel_mgmt.c | 38 ++ drivers/hv/connection.c| 7 ++ drivers/net/hyperv/netvsc.c| 18 -- include/asm-generic/hyperv-tlfs.h | 1 + include/asm-generic/mshyperv.h | 5 include/linux/hyperv.h | 1 + 9 files changed, 107 insertions(+), 2 deletions(-) -- 2.25.1
[PATCH v2 net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer
Pointers to receive-buffer packets sent by Hyper-V are used within the
guest VM. Hyper-V can send packets with erroneous values or modify
packet fields after they are processed by the guest. To defend against
these scenarios, copy (sections of) the incoming packet after validating
their length and offset fields in netvsc_filter_receive(). In this way,
the packet can no longer be modified by the host.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
Changes since v1 [1]:
- copy certain PPIs into the RSC pkt
[1] https://lkml.kernel.org/r/20210126113847.1676-1-parri.and...@gmail.com
drivers/net/hyperv/hyperv_net.h | 93 +++--
drivers/net/hyperv/netvsc.c | 20 +++
drivers/net/hyperv/netvsc_drv.c | 24
drivers/net/hyperv/rndis_filter.c | 99 +--
4 files changed, 150 insertions(+), 86 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 2a87cfa27ac02..e1a497d3c9ba4 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -105,9 +105,43 @@ struct ndis_recv_scale_param { /*
NDIS_RECEIVE_SCALE_PARAMETERS */
u32 processor_masks_entry_size;
};
-/* Fwd declaration */
-struct ndis_tcp_ip_checksum_info;
-struct ndis_pkt_8021q_info;
+struct ndis_tcp_ip_checksum_info {
+ union {
+ struct {
+ u32 is_ipv4:1;
+ u32 is_ipv6:1;
+ u32 tcp_checksum:1;
+ u32 udp_checksum:1;
+ u32 ip_header_checksum:1;
+ u32 reserved:11;
+ u32 tcp_header_offset:10;
+ } transmit;
+ struct {
+ u32 tcp_checksum_failed:1;
+ u32 udp_checksum_failed:1;
+ u32 ip_checksum_failed:1;
+ u32 tcp_checksum_succeeded:1;
+ u32 udp_checksum_succeeded:1;
+ u32 ip_checksum_succeeded:1;
+ u32 loopback:1;
+ u32 tcp_checksum_value_invalid:1;
+ u32 ip_checksum_value_invalid:1;
+ } receive;
+ u32 value;
+ };
+};
+
+struct ndis_pkt_8021q_info {
+ union {
+ struct {
+ u32 pri:3; /* User Priority */
+ u32 cfi:1; /* Canonical Format ID */
+ u32 vlanid:12; /* VLAN ID */
+ u32 reserved:16;
+ };
+ u32 value;
+ };
+};
/*
* Represent netvsc packet which contains 1 RNDIS and 1 ethernet frame
@@ -194,7 +228,8 @@ int netvsc_send(struct net_device *net,
struct sk_buff *skb,
bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
- struct rndis_message *resp);
+ struct rndis_message *resp,
+ void *data);
int netvsc_recv_callback(struct net_device *net,
struct netvsc_device *nvdev,
struct netvsc_channel *nvchan);
@@ -884,9 +919,10 @@ struct multi_recv_comp {
#define NVSP_RSC_MAX 562 /* Max #RSC frags in a vmbus xfer page pkt */
struct nvsc_rsc {
- const struct ndis_pkt_8021q_info *vlan;
- const struct ndis_tcp_ip_checksum_info *csum_info;
- const u32 *hash_info;
+ struct ndis_pkt_8021q_info vlan;
+ struct ndis_tcp_ip_checksum_info csum_info;
+ u32 hash_info;
+ u8 ppi_flags; /* valid/present bits for the above PPIs */
u8 is_last; /* last RNDIS msg in a vmtransfer_page */
u32 cnt; /* #fragments in an RSC packet */
u32 pktlen; /* Full packet length */
@@ -894,6 +930,10 @@ struct nvsc_rsc {
u32 len[NVSP_RSC_MAX];
};
+#define NVSC_RSC_VLAN BIT(0) /* valid/present bit for 'vlan' */
+#define NVSC_RSC_CSUM_INFO BIT(1) /* valid/present bit for 'csum_info' */
+#define NVSC_RSC_HASH_INFO BIT(2) /* valid/present bit for 'hash_info' */
+
struct netvsc_stats {
u64 packets;
u64 bytes;
@@ -1002,6 +1042,7 @@ struct net_device_context {
struct netvsc_channel {
struct vmbus_channel *channel;
struct netvsc_device *net_device;
+ void *recv_buf; /* buffer to copy packets out from the receive buffer */
const struct vmpacket_descriptor *desc;
struct napi_struct napi;
struct multi_send_data msd;
@@ -1234,18 +1275,6 @@ struct rndis_pktinfo_id {
u16 pkt_id;
};
-struct ndis_pkt_8021q_info {
- union {
- struct {
- u32 pri:3; /* User Priority */
- u32 cfi:1; /* Canonical Format ID */
- u32 vlanid:12; /* VLAN ID */
-
Re: [PATCH net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer
On Tue, Jan 26, 2021 at 12:38:47PM +0100, Andrea Parri (Microsoft) wrote: > Pointers to receive-buffer packets sent by Hyper-V are used within the > guest VM. Hyper-V can send packets with erroneous values or modify > packet fields after they are processed by the guest. To defend against > these scenarios, copy (sections of) the incoming packet after validating > their length and offset fields in netvsc_filter_receive(). In this way, > the packet can no longer be modified by the host. > > Reported-by: Juan Vazquez > Signed-off-by: Andrea Parri (Microsoft) > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: net...@vger.kernel.org Please ignore this submission, I'm sending a new version shortly... Sorry for the hassle. Andrea
[PATCH v2 1/4] x86/hyperv: Load/save the Isolation Configuration leaf
If bit 22 of Group B Features is set, the guest has access to the
Isolation Configuration CPUID leaf. On x86, the first four bits
of EAX in this leaf provide the isolation type of the partition;
we entail three isolation types: 'SNP' (hardware-based isolation),
'VBS' (software-based isolation), and 'NONE' (no isolation).
Signed-off-by: Andrea Parri (Microsoft)
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: Arnd Bergmann
Cc: x...@kernel.org
Cc: linux-a...@vger.kernel.org
---
arch/x86/hyperv/hv_init.c | 15 +++
arch/x86/include/asm/hyperv-tlfs.h | 15 +++
arch/x86/kernel/cpu/mshyperv.c | 9 +
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 5 +
5 files changed, 45 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index e04d90af4c27c..dc94e95c57b98 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -528,3 +529,17 @@ bool hv_is_hibernation_supported(void)
return acpi_sleep_state_supported(ACPI_STATE_S4);
}
EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
+
+enum hv_isolation_type hv_get_isolation_type(void)
+{
+ if (!(ms_hyperv.hypercalls_features & HV_ISOLATION))
+ return HV_ISOLATION_TYPE_NONE;
+ return FIELD_GET(HV_ISOLATION_TYPE, ms_hyperv.isolation_config_b);
+}
+EXPORT_SYMBOL_GPL(hv_get_isolation_type);
+
+bool hv_is_isolation_supported(void)
+{
+ return hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;
+}
+EXPORT_SYMBOL_GPL(hv_is_isolation_supported);
diff --git a/arch/x86/include/asm/hyperv-tlfs.h
b/arch/x86/include/asm/hyperv-tlfs.h
index 6bf42aed387e3..6aed936e5e962 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -22,6 +22,7 @@
#define HYPERV_CPUID_ENLIGHTMENT_INFO 0x4004
#define HYPERV_CPUID_IMPLEMENT_LIMITS 0x4005
#define HYPERV_CPUID_NESTED_FEATURES 0x400A
+#define HYPERV_CPUID_ISOLATION_CONFIG 0x400C
#define HYPERV_CPUID_VIRT_STACK_INTERFACE 0x4081
#define HYPERV_VS_INTERFACE_EAX_SIGNATURE 0x31235356 /* "VS#1" */
@@ -122,6 +123,20 @@
#define HV_X64_NESTED_GUEST_MAPPING_FLUSH BIT(18)
#define HV_X64_NESTED_MSR_BITMAP BIT(19)
+/* HYPERV_CPUID_ISOLATION_CONFIG.EAX bits. */
+#define HV_PARAVISOR_PRESENT BIT(0)
+
+/* HYPERV_CPUID_ISOLATION_CONFIG.EBX bits. */
+#define HV_ISOLATION_TYPE GENMASK(3, 0)
+#define HV_SHARED_GPA_BOUNDARY_ACTIVE BIT(5)
+#define HV_SHARED_GPA_BOUNDARY_BITSGENMASK(11, 6)
+
+enum hv_isolation_type {
+ HV_ISOLATION_TYPE_NONE = 0,
+ HV_ISOLATION_TYPE_VBS = 1,
+ HV_ISOLATION_TYPE_SNP = 2
+};
+
/* Hyper-V specific model specific registers (MSRs) */
/* MSR used to identify the guest OS. */
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index f628e3dc150f3..0d4aaf6694d01 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -225,6 +225,7 @@ static void __init ms_hyperv_init_platform(void)
* Extract the features and hints
*/
ms_hyperv.features = cpuid_eax(HYPERV_CPUID_FEATURES);
+ ms_hyperv.hypercalls_features = cpuid_ebx(HYPERV_CPUID_FEATURES);
ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
ms_hyperv.hints= cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
@@ -259,6 +260,14 @@ static void __init ms_hyperv_init_platform(void)
x86_platform.calibrate_cpu = hv_get_tsc_khz;
}
+ if (ms_hyperv.hypercalls_features & HV_ISOLATION) {
+ ms_hyperv.isolation_config_a =
cpuid_eax(HYPERV_CPUID_ISOLATION_CONFIG);
+ ms_hyperv.isolation_config_b =
cpuid_ebx(HYPERV_CPUID_ISOLATION_CONFIG);
+
+ pr_info("Hyper-V: Isolation Config: GroupA 0x%x, GroupB 0x%x\n",
+ ms_hyperv.isolation_config_a,
ms_hyperv.isolation_config_b);
+ }
+
if (ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED) {
ms_hyperv.nested_features =
cpuid_eax(HYPERV_CPUID_NESTED_FEATURES);
diff --git a/include/asm-generic/hyperv-tlfs.h
b/include/asm-generic/hyperv-tlfs.h
index e73a11850055c..20d3cd9502043 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -89,6 +89,7 @@
#define HV_ACCESS_STATSBIT(8)
#define HV_DEBUGGING BIT(11)
#define HV_CPU_POWER_MANAGEMENTBIT(12)
+#define HV_ISOLATION BIT(22)
/*
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index c5779968417
[PATCH v2 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests
Changes since v1 [1]: - improve/add logging (Haiyang Zhang) - move NVSC version check after version negotiation (Haiyang Zhang) [1] https://lkml.kernel.org/r/20210119175841.22248-1-parri.and...@gmail.com Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: "H. Peter Anvin" Cc: Arnd Bergmann Cc: "David S. Miller" Cc: Jakub Kicinski Cc: x...@kernel.org Cc: linux-a...@vger.kernel.org Cc: net...@vger.kernel.org Andrea Parri (Microsoft) (4): x86/hyperv: Load/save the Isolation Configuration leaf Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests hv_netvsc: Restrict configurations on isolated guests arch/x86/hyperv/hv_init.c | 15 + arch/x86/include/asm/hyperv-tlfs.h | 15 + arch/x86/kernel/cpu/mshyperv.c | 9 drivers/hv/channel_mgmt.c | 36 ++ drivers/hv/connection.c| 13 +++ drivers/net/hyperv/netvsc.c| 27 +++--- include/asm-generic/hyperv-tlfs.h | 1 + include/asm-generic/mshyperv.h | 5 + include/linux/hyperv.h | 1 + 9 files changed, 119 insertions(+), 3 deletions(-) -- 2.25.1
[PATCH v2 4/4] hv_netvsc: Restrict configurations on isolated guests
Restrict the NVSP protocol version(s) that will be negotiated with the
host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
isolated. Moreover, do not advertise the SR-IOV capability and ignore
NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests, which
are not supposed to support SR-IOV. This reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
Acked-by: Jakub Kicinski
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
drivers/net/hyperv/netvsc.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..afd92b4aa21fe 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -22,6 +22,7 @@
#include
#include
+#include
#include "hyperv_net.h"
#include "netvsc_trace.h"
@@ -544,7 +545,10 @@ static int negotiate_nvsp_ver(struct hv_device *device,
init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
- init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
+ if (!hv_is_isolation_supported())
+
init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
+ else
+ netdev_info(ndev, "SR-IOV not advertised by guests on
the host supporting isolation\n");
/* Teaming bit is needed to receive link speed updates */
init_packet->msg.v2_msg.send_ndis_config.capability.teaming = 1;
@@ -563,6 +567,13 @@ static int negotiate_nvsp_ver(struct hv_device *device,
return ret;
}
+static bool nvsp_is_valid_version(u32 version)
+{
+ if (hv_is_isolation_supported())
+ return version >= NVSP_PROTOCOL_VERSION_61;
+ return true;
+}
+
static int netvsc_connect_vsp(struct hv_device *device,
struct netvsc_device *net_device,
const struct netvsc_device_info *device_info)
@@ -579,12 +590,19 @@ static int netvsc_connect_vsp(struct hv_device *device,
init_packet = _device->channel_init_pkt;
/* Negotiate the latest NVSP protocol supported */
- for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
+ for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
if (negotiate_nvsp_ver(device, net_device, init_packet,
ver_list[i]) == 0) {
+ if (!nvsp_is_valid_version(ver_list[i])) {
+ netdev_err(ndev, "Invalid NVSP version 0x%x
(expected >= 0x%x) from the host with isolation supported\n",
+ ver_list[i],
NVSP_PROTOCOL_VERSION_61);
+ ret = -EPROTO;
+ goto cleanup;
+ }
net_device->nvsp_version = ver_list[i];
break;
}
+ }
if (i < 0) {
ret = -EPROTO;
@@ -1357,7 +1375,10 @@ static void netvsc_receive_inband(struct net_device
*ndev,
break;
case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
- netvsc_send_vf(ndev, nvmsg, msglen);
+ if (!hv_is_isolation_supported())
+ netvsc_send_vf(ndev, nvmsg, msglen);
+ else
+ netdev_err(ndev, "Ignore VF_ASSOCIATION msg from the
host supporting isolation\n");
break;
}
}
--
2.25.1
[PATCH v2 2/4] Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests
Only the VSCs or ICs that have been hardened and that are critical for
the successful adoption of Confidential VMs should be allowed if the
guest is running isolated. This change reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel_mgmt.c | 36
include/linux/hyperv.h| 1 +
2 files changed, 37 insertions(+)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..774ee19e3e90d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -31,101 +31,118 @@ const struct vmbus_device vmbus_devs[] = {
{ .dev_type = HV_IDE,
HV_IDE_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* SCSI */
{ .dev_type = HV_SCSI,
HV_SCSI_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Fibre Channel */
{ .dev_type = HV_FC,
HV_SYNTHFC_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* Synthetic NIC */
{ .dev_type = HV_NIC,
HV_NIC_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Network Direct */
{ .dev_type = HV_ND,
HV_ND_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* PCIE */
{ .dev_type = HV_PCIE,
HV_PCIE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Frame Buffer */
{ .dev_type = HV_FB,
HV_SYNTHVID_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Keyboard */
{ .dev_type = HV_KBD,
HV_KBD_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic MOUSE */
{ .dev_type = HV_MOUSE,
HV_MOUSE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* KVP */
{ .dev_type = HV_KVP,
HV_KVP_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Time Synch */
{ .dev_type = HV_TS,
HV_TS_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Heartbeat */
{ .dev_type = HV_HB,
HV_HEART_BEAT_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Shutdown */
{ .dev_type = HV_SHUTDOWN,
HV_SHUTDOWN_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* File copy */
{ .dev_type = HV_FCOPY,
HV_FCOPY_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Backup */
{ .dev_type = HV_BACKUP,
HV_VSS_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Dynamic Memory */
{ .dev_type = HV_DM,
HV_DM_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Unknown GUID */
{ .dev_type = HV_UNKNOWN,
.perf_device = false,
+ .allowed_in_isolated = false,
},
};
@@ -903,6 +920,20 @@ find_primary_channel_by_offer(const struct
vmbus_channel_offer_channel *offer)
return channel;
}
+static bool vmbus_is_valid_device(const guid_t *guid)
+{
+ u16 i;
+
+ if (!hv_is_isolation_supported())
+ return true;
+
+ for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
+ if (guid_equal(guid, _devs[i].guid))
+ return vmbus_devs[i].allowed_in_isolated;
+ }
+ return false;
+}
+
/*
* vmbus_onoffer - Handler for channel offers from vmbus in parent partition.
*
@@ -917,6 +948,11 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
trace_vmbus_onoffer(offer);
+ if (!vmbus_is_valid_device(>offer.if_type)) {
+ atomic_dec(_connection.offer_in_progress);
+ return;
+ }
+
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index f0d48a368f131..e3426f8c12db9 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -789,6 +789,7 @@ struct vmbus_device {
u16 dev_type;
guid_t guid;
bool perf_device;
+ bool allowed_in_isolated;
};
#define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
--
2.25.1
[PATCH v2 3/4] Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests
Restrict the protocol version(s) that will be negotiated with the host
to be 5.2 or greater if the guest is running isolated. This reduces the
footprint of the code that will be exercised by Confidential VMs and
hence the exposure to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/connection.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 11170d9a2e1a5..bcf4d7def6838 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -66,6 +66,13 @@ module_param(max_version, uint, S_IRUGO);
MODULE_PARM_DESC(max_version,
"Maximal VMBus protocol version which can be negotiated");
+static bool vmbus_is_valid_version(u32 version)
+{
+ if (hv_is_isolation_supported())
+ return version >= VERSION_WIN10_V5_2;
+ return true;
+}
+
int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
{
int ret = 0;
@@ -233,6 +240,12 @@ int vmbus_connect(void)
goto cleanup;
version = vmbus_versions[i];
+
+ if (!vmbus_is_valid_version(version)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
+
if (version > max_version)
continue;
--
2.25.1
[PATCH net-next] hv_netvsc: Copy packets sent by Hyper-V out of the receive buffer
Pointers to receive-buffer packets sent by Hyper-V are used within the
guest VM. Hyper-V can send packets with erroneous values or modify
packet fields after they are processed by the guest. To defend against
these scenarios, copy (sections of) the incoming packet after validating
their length and offset fields in netvsc_filter_receive(). In this way,
the packet can no longer be modified by the host.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
drivers/net/hyperv/hyperv_net.h | 4 +-
drivers/net/hyperv/netvsc.c | 20 +
drivers/net/hyperv/netvsc_drv.c | 9 ++--
drivers/net/hyperv/rndis_filter.c | 74 +++
4 files changed, 75 insertions(+), 32 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 2a87cfa27ac02..2c0ab80d8ae4e 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -194,7 +194,8 @@ int netvsc_send(struct net_device *net,
struct sk_buff *skb,
bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
- struct rndis_message *resp);
+ struct rndis_message *resp,
+ void *data);
int netvsc_recv_callback(struct net_device *net,
struct netvsc_device *nvdev,
struct netvsc_channel *nvchan);
@@ -1002,6 +1003,7 @@ struct net_device_context {
struct netvsc_channel {
struct vmbus_channel *channel;
struct netvsc_device *net_device;
+ void *recv_buf; /* buffer to copy packets out from the receive buffer */
const struct vmpacket_descriptor *desc;
struct napi_struct napi;
struct multi_send_data msd;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 6184e99c7f31f..0fba8257fc119 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -131,6 +131,7 @@ static void free_netvsc_device(struct rcu_head *head)
for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
xdp_rxq_info_unreg(>chan_table[i].xdp_rxq);
+ kfree(nvdev->chan_table[i].recv_buf);
vfree(nvdev->chan_table[i].mrc.slots);
}
@@ -1284,6 +1285,19 @@ static int netvsc_receive(struct net_device *ndev,
continue;
}
+ /* We're going to copy (sections of) the packet into
nvchan->recv_buf;
+* make sure that nvchan->recv_buf is large enough to hold the
packet.
+*/
+ if (unlikely(buflen > net_device->recv_section_size)) {
+ nvchan->rsc.cnt = 0;
+ status = NVSP_STAT_FAIL;
+ netif_err(net_device_ctx, rx_err, ndev,
+ "Packet too big: buflen=%u
recv_section_size=%u\n",
+ buflen, net_device->recv_section_size);
+
+ continue;
+ }
+
data = recv_buf + offset;
nvchan->rsc.is_last = (i == count - 1);
@@ -1535,6 +1549,12 @@ struct netvsc_device *netvsc_device_add(struct hv_device
*device,
for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
struct netvsc_channel *nvchan = _device->chan_table[i];
+ nvchan->recv_buf = kzalloc(device_info->recv_section_size,
GFP_KERNEL);
+ if (nvchan->recv_buf == NULL) {
+ ret = -ENOMEM;
+ goto cleanup2;
+ }
+
nvchan->channel = device->channel;
nvchan->net_device = net_device;
u64_stats_init(>tx_stats.syncp);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index ac20c432d4d8f..1ec27bbe267da 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -743,7 +743,8 @@ static netdev_tx_t netvsc_start_xmit(struct sk_buff *skb,
* netvsc_linkstatus_callback - Link up/down notification
*/
void netvsc_linkstatus_callback(struct net_device *net,
- struct rndis_message *resp)
+ struct rndis_message *resp,
+ void *data)
{
struct rndis_indicate_status *indicate = >msg.indicate_status;
struct net_device_context *ndev_ctx = netdev_priv(net);
@@ -757,6 +758,9 @@ void netvsc_linkstatus_callback(struct net_device *net,
return;
}
+ /* Copy the RNDIS indicate status into nvchan->recv_buf */
+ memcpy(indicate, data + RNDIS_HEADER_SIZE, sizeof(*indicate));
+
/* Update the physical link speed when changing to another vSwitch */
if (indicate->status
Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
> > > > @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> > > > *device,
> > > > init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q =
> > > > 1;
> > > >
> > > > if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> > > > -
> > > > init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> > > > 1;
> > > > + if (!hv_is_isolation_supported())
> > > > + init_packet-
> > > > >msg.v2_msg.send_ndis_config.capability.sriov = 1;
> > >
> > > Please also add a log there stating we don't support sriov in this case.
> > Otherwise,
> > > customers will ask why vf not showing up.
> >
> > IIUC, you're suggesting that I append something like:
> >
> > + else
> > + netdev_info(ndev, "SR-IOV not advertised: isolation
> > supported\n");
> >
> > I've added this locally; please let me know if you had something else
> > /better in mind.
>
> This message explains the failure reason better:
> "SR-IOV not advertised by guests on the host supporting isolation"
Applied.
> > > > @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> > > > *device,
> > > > init_packet = _device->channel_init_pkt;
> > > >
> > > > /* Negotiate the latest NVSP protocol supported */
> > > > - for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> > > > + for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> > > > + if (!nvsp_is_valid_version(ver_list[i])) {
> > > > + ret = -EPROTO;
> > > > + goto cleanup;
> > > > + }
> > >
> > > This code can catch the invalid, but cannot get the current host nvsp
> > version.
> > > I'd suggest move this check after version negotiation is done. So we can
> > > log
> > what's
> > > the current host nvsp version, and why we fail it (the expected nvsp ver).
> >
> > Mmh, invalid versions are not negotiated. How about I simply add the
> > following logging right before the above 'ret = -EPROTO' say?
> >
> > + netdev_err(ndev, "Invalid NVSP version %x
> > (expected >= %x): isolation supported\n",
> > + ver_list[i], NVSP_PROTOCOL_VERSION_61);
> >
> > (or something along these lines)
>
> The negotiation process runs from the latest to oldest. If the host is 5,
> your code
> will fail before trying v6.0, and log:
> "Invalid NVSP version 6 (expected >= 60001): isolation supported"
> This will make user think the NVSP version is 6.0.
>
> Since you will let the NIC fail and cleanup, there is no harm to check the
> version
> after negotiation. And this case is unexpected from a "normal" host. So I
> suggest
> move the check after negotiation is done, then we know the actual host nvsp
> version that causing this issue. And we can bring the accurate info to host
> team
> for better diagnosability.
Fair enough, will do.
> Please point out this invalid version is caused by the host side, like this:
> "Invalid NVSP version 0x5 (expected >= 0x60001) from the host with
> isolation support"
> Also please use "0x%x" for hexadecimal numbers.
Sure.
> > > > @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> > > > net_device *ndev,
> > > > break;
> > > >
> > > > case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> > > > - netvsc_send_vf(ndev, nvmsg, msglen);
> > > > + if (!hv_is_isolation_supported())
> > > > + netvsc_send_vf(ndev, nvmsg, msglen);
> > >
> > > When the driver doesn't advertise SRIOV, this message is not expected.
> > > Instead of ignore silently, we should log an error.
> >
> > I've appended:
> >
> > + else
> > + netdev_err(ndev, "Unexpected VF message:
> > isolation supported\n");
>
> Please log the msg type:
> "Ignore VF_ASSOCIATION msg from the host supporting isolation"
Applied.
Thanks,
Andrea
Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
> > @@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device
> > *device,
> > init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
> >
> > if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
> > - init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
> > 1;
> > + if (!hv_is_isolation_supported())
> > + init_packet-
> > >msg.v2_msg.send_ndis_config.capability.sriov = 1;
>
> Please also add a log there stating we don't support sriov in this case.
> Otherwise,
> customers will ask why vf not showing up.
IIUC, you're suggesting that I append something like:
+ else
+ netdev_info(ndev, "SR-IOV not advertised: isolation
supported\n");
I've added this locally; please let me know if you had something else
/better in mind.
> > @@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device
> > *device,
> > return ret;
> > }
> >
> > +static bool nvsp_is_valid_version(u32 version)
> > +{
> > + if (hv_is_isolation_supported())
> > + return version >= NVSP_PROTOCOL_VERSION_61;
> > + return true;
> Hosts support isolation should run nvsp 6.1+. This error is not expected.
> Instead of fail silently, we should log an error to explain why it's failed,
> and the current version and expected version.
Please see my next comment below.
> > +}
> > +
> > static int netvsc_connect_vsp(struct hv_device *device,
> > struct netvsc_device *net_device,
> > const struct netvsc_device_info *device_info)
> > @@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device
> > *device,
> > init_packet = _device->channel_init_pkt;
> >
> > /* Negotiate the latest NVSP protocol supported */
> > - for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
> > + for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
> > + if (!nvsp_is_valid_version(ver_list[i])) {
> > + ret = -EPROTO;
> > + goto cleanup;
> > + }
>
> This code can catch the invalid, but cannot get the current host nvsp version.
> I'd suggest move this check after version negotiation is done. So we can log
> what's
> the current host nvsp version, and why we fail it (the expected nvsp ver).
Mmh, invalid versions are not negotiated. How about I simply add the
following logging right before the above 'ret = -EPROTO' say?
+ netdev_err(ndev, "Invalid NVSP version %x (expected >=
%x): isolation supported\n",
+ ver_list[i], NVSP_PROTOCOL_VERSION_61);
(or something along these lines)
> > @@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct
> > net_device *ndev,
> > break;
> >
> > case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
> > - netvsc_send_vf(ndev, nvmsg, msglen);
> > + if (!hv_is_isolation_supported())
> > + netvsc_send_vf(ndev, nvmsg, msglen);
>
> When the driver doesn't advertise SRIOV, this message is not expected.
> Instead of ignore silently, we should log an error.
I've appended:
+ else
+ netdev_err(ndev, "Unexpected VF message: isolation
supported\n");
Please let me know if I got this wrong.
Thanks,
Andrea
[PATCH 0/4] Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests
Hi all, To reduce the footprint of the code that will be exercised, and hence the exposure to bugs and vulnerabilities, restrict configurations and devices on 'isolated' VMs. Specs of the Isolation Configuration leaf (cf. patch #1) were derived from internal discussions with the Hyper-V team and, AFAICT, they are not publicly available yet. The series has some minor/naming conflict with on-going work aimed at enabling SNP VMs on Hyper-V[1]; such conflicts can be addressed later at the right time. Applies to hyperv-next. Thanks, Andrea [1] https://github.com/lantianyu/linux # cvm Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: "H. Peter Anvin" Cc: Arnd Bergmann Cc: "David S. Miller" Cc: Jakub Kicinski Cc: x...@kernel.org Cc: linux-a...@vger.kernel.org Cc: net...@vger.kernel.org Andrea Parri (Microsoft) (4): x86/hyperv: Load/save the Isolation Configuration leaf Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests hv_netvsc: Restrict configurations on isolated guests arch/x86/hyperv/hv_init.c | 15 + arch/x86/include/asm/hyperv-tlfs.h | 15 + arch/x86/kernel/cpu/mshyperv.c | 9 drivers/hv/channel_mgmt.c | 36 ++ drivers/hv/connection.c| 13 +++ drivers/net/hyperv/netvsc.c| 21 ++--- include/asm-generic/hyperv-tlfs.h | 1 + include/asm-generic/mshyperv.h | 5 + include/linux/hyperv.h | 1 + 9 files changed, 113 insertions(+), 3 deletions(-) -- 2.25.1
[PATCH 2/4] Drivers: hv: vmbus: Restrict vmbus_devices on isolated guests
Only the VSCs or ICs that have been hardened and that are critical for
the successful adoption of Confidential VMs should be allowed if the
guest is running isolated. This change reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel_mgmt.c | 36
include/linux/hyperv.h| 1 +
2 files changed, 37 insertions(+)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..774ee19e3e90d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -31,101 +31,118 @@ const struct vmbus_device vmbus_devs[] = {
{ .dev_type = HV_IDE,
HV_IDE_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* SCSI */
{ .dev_type = HV_SCSI,
HV_SCSI_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Fibre Channel */
{ .dev_type = HV_FC,
HV_SYNTHFC_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* Synthetic NIC */
{ .dev_type = HV_NIC,
HV_NIC_GUID,
.perf_device = true,
+ .allowed_in_isolated = true,
},
/* Network Direct */
{ .dev_type = HV_ND,
HV_ND_GUID,
.perf_device = true,
+ .allowed_in_isolated = false,
},
/* PCIE */
{ .dev_type = HV_PCIE,
HV_PCIE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Frame Buffer */
{ .dev_type = HV_FB,
HV_SYNTHVID_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic Keyboard */
{ .dev_type = HV_KBD,
HV_KBD_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Synthetic MOUSE */
{ .dev_type = HV_MOUSE,
HV_MOUSE_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* KVP */
{ .dev_type = HV_KVP,
HV_KVP_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Time Synch */
{ .dev_type = HV_TS,
HV_TS_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Heartbeat */
{ .dev_type = HV_HB,
HV_HEART_BEAT_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* Shutdown */
{ .dev_type = HV_SHUTDOWN,
HV_SHUTDOWN_GUID,
.perf_device = false,
+ .allowed_in_isolated = true,
},
/* File copy */
{ .dev_type = HV_FCOPY,
HV_FCOPY_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Backup */
{ .dev_type = HV_BACKUP,
HV_VSS_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Dynamic Memory */
{ .dev_type = HV_DM,
HV_DM_GUID,
.perf_device = false,
+ .allowed_in_isolated = false,
},
/* Unknown GUID */
{ .dev_type = HV_UNKNOWN,
.perf_device = false,
+ .allowed_in_isolated = false,
},
};
@@ -903,6 +920,20 @@ find_primary_channel_by_offer(const struct
vmbus_channel_offer_channel *offer)
return channel;
}
+static bool vmbus_is_valid_device(const guid_t *guid)
+{
+ u16 i;
+
+ if (!hv_is_isolation_supported())
+ return true;
+
+ for (i = 0; i < ARRAY_SIZE(vmbus_devs); i++) {
+ if (guid_equal(guid, _devs[i].guid))
+ return vmbus_devs[i].allowed_in_isolated;
+ }
+ return false;
+}
+
/*
* vmbus_onoffer - Handler for channel offers from vmbus in parent partition.
*
@@ -917,6 +948,11 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
trace_vmbus_onoffer(offer);
+ if (!vmbus_is_valid_device(>offer.if_type)) {
+ atomic_dec(_connection.offer_in_progress);
+ return;
+ }
+
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index f0d48a368f131..e3426f8c12db9 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -789,6 +789,7 @@ struct vmbus_device {
u16 dev_type;
guid_t guid;
bool perf_device;
+ bool allowed_in_isolated;
};
#define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
--
2.25.1
[PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests
Restrict the NVSP protocol version(s) that will be negotiated with the
host to be NVSP_PROTOCOL_VERSION_61 or greater if the guest is running
isolated. Moreover, do not advertise the SR-IOV capability and ignore
NVSP_MSG_4_TYPE_SEND_VF_ASSOCIATION messages in isolated guests, which
are not supposed to support SR-IOV. This reduces the footprint of the
code that will be exercised by Confidential VMs and hence the exposure
to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
drivers/net/hyperv/netvsc.c | 21 ++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..8027d553cb67d 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -22,6 +22,7 @@
#include
#include
+#include
#include "hyperv_net.h"
#include "netvsc_trace.h"
@@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device *device,
init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;
if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
- init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
+ if (!hv_is_isolation_supported())
+
init_packet->msg.v2_msg.send_ndis_config.capability.sriov = 1;
/* Teaming bit is needed to receive link speed updates */
init_packet->msg.v2_msg.send_ndis_config.capability.teaming = 1;
@@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device *device,
return ret;
}
+static bool nvsp_is_valid_version(u32 version)
+{
+ if (hv_is_isolation_supported())
+ return version >= NVSP_PROTOCOL_VERSION_61;
+ return true;
+}
+
static int netvsc_connect_vsp(struct hv_device *device,
struct netvsc_device *net_device,
const struct netvsc_device_info *device_info)
@@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device *device,
init_packet = _device->channel_init_pkt;
/* Negotiate the latest NVSP protocol supported */
- for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
+ for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
+ if (!nvsp_is_valid_version(ver_list[i])) {
+ ret = -EPROTO;
+ goto cleanup;
+ }
if (negotiate_nvsp_ver(device, net_device, init_packet,
ver_list[i]) == 0) {
net_device->nvsp_version = ver_list[i];
break;
}
+ }
if (i < 0) {
ret = -EPROTO;
@@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct net_device *ndev,
break;
case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
- netvsc_send_vf(ndev, nvmsg, msglen);
+ if (!hv_is_isolation_supported())
+ netvsc_send_vf(ndev, nvmsg, msglen);
break;
}
}
--
2.25.1
[PATCH 1/4] x86/hyperv: Load/save the Isolation Configuration leaf
If bit 22 of Group B Features is set, the guest has access to the
Isolation Configuration CPUID leaf. On x86, the first four bits
of EAX in this leaf provide the isolation type of the partition;
we entail three isolation types: 'SNP' (hardware-based isolation),
'VBS' (software-based isolation), and 'NONE' (no isolation).
Signed-off-by: Andrea Parri (Microsoft)
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: "H. Peter Anvin"
Cc: Arnd Bergmann
Cc: x...@kernel.org
Cc: linux-a...@vger.kernel.org
---
arch/x86/hyperv/hv_init.c | 15 +++
arch/x86/include/asm/hyperv-tlfs.h | 15 +++
arch/x86/kernel/cpu/mshyperv.c | 9 +
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 5 +
5 files changed, 45 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index e04d90af4c27c..dc94e95c57b98 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -528,3 +529,17 @@ bool hv_is_hibernation_supported(void)
return acpi_sleep_state_supported(ACPI_STATE_S4);
}
EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
+
+enum hv_isolation_type hv_get_isolation_type(void)
+{
+ if (!(ms_hyperv.hypercalls_features & HV_ISOLATION))
+ return HV_ISOLATION_TYPE_NONE;
+ return FIELD_GET(HV_ISOLATION_TYPE, ms_hyperv.isolation_config_b);
+}
+EXPORT_SYMBOL_GPL(hv_get_isolation_type);
+
+bool hv_is_isolation_supported(void)
+{
+ return hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;
+}
+EXPORT_SYMBOL_GPL(hv_is_isolation_supported);
diff --git a/arch/x86/include/asm/hyperv-tlfs.h
b/arch/x86/include/asm/hyperv-tlfs.h
index 6bf42aed387e3..6aed936e5e962 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -22,6 +22,7 @@
#define HYPERV_CPUID_ENLIGHTMENT_INFO 0x4004
#define HYPERV_CPUID_IMPLEMENT_LIMITS 0x4005
#define HYPERV_CPUID_NESTED_FEATURES 0x400A
+#define HYPERV_CPUID_ISOLATION_CONFIG 0x400C
#define HYPERV_CPUID_VIRT_STACK_INTERFACE 0x4081
#define HYPERV_VS_INTERFACE_EAX_SIGNATURE 0x31235356 /* "VS#1" */
@@ -122,6 +123,20 @@
#define HV_X64_NESTED_GUEST_MAPPING_FLUSH BIT(18)
#define HV_X64_NESTED_MSR_BITMAP BIT(19)
+/* HYPERV_CPUID_ISOLATION_CONFIG.EAX bits. */
+#define HV_PARAVISOR_PRESENT BIT(0)
+
+/* HYPERV_CPUID_ISOLATION_CONFIG.EBX bits. */
+#define HV_ISOLATION_TYPE GENMASK(3, 0)
+#define HV_SHARED_GPA_BOUNDARY_ACTIVE BIT(5)
+#define HV_SHARED_GPA_BOUNDARY_BITSGENMASK(11, 6)
+
+enum hv_isolation_type {
+ HV_ISOLATION_TYPE_NONE = 0,
+ HV_ISOLATION_TYPE_VBS = 1,
+ HV_ISOLATION_TYPE_SNP = 2
+};
+
/* Hyper-V specific model specific registers (MSRs) */
/* MSR used to identify the guest OS. */
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index f628e3dc150f3..0d4aaf6694d01 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -225,6 +225,7 @@ static void __init ms_hyperv_init_platform(void)
* Extract the features and hints
*/
ms_hyperv.features = cpuid_eax(HYPERV_CPUID_FEATURES);
+ ms_hyperv.hypercalls_features = cpuid_ebx(HYPERV_CPUID_FEATURES);
ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
ms_hyperv.hints= cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
@@ -259,6 +260,14 @@ static void __init ms_hyperv_init_platform(void)
x86_platform.calibrate_cpu = hv_get_tsc_khz;
}
+ if (ms_hyperv.hypercalls_features & HV_ISOLATION) {
+ ms_hyperv.isolation_config_a =
cpuid_eax(HYPERV_CPUID_ISOLATION_CONFIG);
+ ms_hyperv.isolation_config_b =
cpuid_ebx(HYPERV_CPUID_ISOLATION_CONFIG);
+
+ pr_info("Hyper-V: Isolation Config: GroupA 0x%x, GroupB 0x%x\n",
+ ms_hyperv.isolation_config_a,
ms_hyperv.isolation_config_b);
+ }
+
if (ms_hyperv.hints & HV_X64_ENLIGHTENED_VMCS_RECOMMENDED) {
ms_hyperv.nested_features =
cpuid_eax(HYPERV_CPUID_NESTED_FEATURES);
diff --git a/include/asm-generic/hyperv-tlfs.h
b/include/asm-generic/hyperv-tlfs.h
index e73a11850055c..20d3cd9502043 100644
--- a/include/asm-generic/hyperv-tlfs.h
+++ b/include/asm-generic/hyperv-tlfs.h
@@ -89,6 +89,7 @@
#define HV_ACCESS_STATSBIT(8)
#define HV_DEBUGGING BIT(11)
#define HV_CPU_POWER_MANAGEMENTBIT(12)
+#define HV_ISOLATION BIT(22)
/*
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index c5779968417
[PATCH 3/4] Drivers: hv: vmbus: Enforce 'VMBus version >= 5.2' on isolated guests
Restrict the protocol version(s) that will be negotiated with the host
to be 5.2 or greater if the guest is running isolated. This reduces the
footprint of the code that will be exercised by Confidential VMs and
hence the exposure to bugs and vulnerabilities.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/connection.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 11170d9a2e1a5..bcf4d7def6838 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -66,6 +66,13 @@ module_param(max_version, uint, S_IRUGO);
MODULE_PARM_DESC(max_version,
"Maximal VMBus protocol version which can be negotiated");
+static bool vmbus_is_valid_version(u32 version)
+{
+ if (hv_is_isolation_supported())
+ return version >= VERSION_WIN10_V5_2;
+ return true;
+}
+
int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
{
int ret = 0;
@@ -233,6 +240,12 @@ int vmbus_connect(void)
goto cleanup;
version = vmbus_versions[i];
+
+ if (!vmbus_is_valid_version(version)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
+
if (version > max_version)
continue;
--
2.25.1
Re: [PATCH v2] hv_netvsc: Add (more) validation for untrusted Hyper-V values
On Sun, Jan 17, 2021 at 03:10:32PM +, Wei Liu wrote: > On Sat, Jan 16, 2021 at 02:02:01PM +0100, Andrea Parri wrote: > > On Fri, Jan 15, 2021 at 08:30:22PM -0800, Jakub Kicinski wrote: > > > On Thu, 14 Jan 2021 21:26:28 +0100 Andrea Parri (Microsoft) wrote: > > > > For additional robustness in the face of Hyper-V errors or malicious > > > > behavior, validate all values that originate from packets that Hyper-V > > > > has sent to the guest. Ensure that invalid values cannot cause indexing > > > > off the end of an array, or subvert an existing validation via integer > > > > overflow. Ensure that outgoing packets do not have any leftover guest > > > > memory that has not been zeroed out. > > > > > > > > Reported-by: Juan Vazquez > > > > Signed-off-by: Andrea Parri (Microsoft) > > > > Cc: "David S. Miller" > > > > Cc: Jakub Kicinski > > > > Cc: Alexei Starovoitov > > > > Cc: Daniel Borkmann > > > > Cc: Andrii Nakryiko > > > > Cc: Martin KaFai Lau > > > > Cc: Song Liu > > > > Cc: Yonghong Song > > > > Cc: John Fastabend > > > > Cc: KP Singh > > > > Cc: net...@vger.kernel.org > > > > Cc: b...@vger.kernel.org > > > > --- > > > > Applies to 5.11-rc3 (and hyperv-next). > > > > > > So this is for hyperv-next or should we take it via netdev trees? > > > > No preference, either way is good for me. > > To be clear: There is no dependency on any patch in hyperv-next, right? > > That's my understanding, but I would like to confirm it. Well, I wrote that this *applies* to hyperv-next... but that's indeed the only 'dependency' I can think of. Hope this helps. Thanks, Andrea
Re: [PATCH v2] hv_netvsc: Add (more) validation for untrusted Hyper-V values
On Fri, Jan 15, 2021 at 08:30:22PM -0800, Jakub Kicinski wrote: > On Thu, 14 Jan 2021 21:26:28 +0100 Andrea Parri (Microsoft) wrote: > > For additional robustness in the face of Hyper-V errors or malicious > > behavior, validate all values that originate from packets that Hyper-V > > has sent to the guest. Ensure that invalid values cannot cause indexing > > off the end of an array, or subvert an existing validation via integer > > overflow. Ensure that outgoing packets do not have any leftover guest > > memory that has not been zeroed out. > > > > Reported-by: Juan Vazquez > > Signed-off-by: Andrea Parri (Microsoft) > > Cc: "David S. Miller" > > Cc: Jakub Kicinski > > Cc: Alexei Starovoitov > > Cc: Daniel Borkmann > > Cc: Andrii Nakryiko > > Cc: Martin KaFai Lau > > Cc: Song Liu > > Cc: Yonghong Song > > Cc: John Fastabend > > Cc: KP Singh > > Cc: net...@vger.kernel.org > > Cc: b...@vger.kernel.org > > --- > > Applies to 5.11-rc3 (and hyperv-next). > > So this is for hyperv-next or should we take it via netdev trees? No preference, either way is good for me. Thanks, Andrea
[PATCH v2] hv_netvsc: Add (more) validation for untrusted Hyper-V values
For additional robustness in the face of Hyper-V errors or malicious
behavior, validate all values that originate from packets that Hyper-V
has sent to the guest. Ensure that invalid values cannot cause indexing
off the end of an array, or subvert an existing validation via integer
overflow. Ensure that outgoing packets do not have any leftover guest
memory that has not been zeroed out.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Alexei Starovoitov
Cc: Daniel Borkmann
Cc: Andrii Nakryiko
Cc: Martin KaFai Lau
Cc: Song Liu
Cc: Yonghong Song
Cc: John Fastabend
Cc: KP Singh
Cc: net...@vger.kernel.org
Cc: b...@vger.kernel.org
---
Applies to 5.11-rc3 (and hyperv-next).
Changes since v1 (Juan Vazquez):
- Improve validation in rndis_set_link_state() and rndis_get_ppi()
- Remove memory/skb leak in netvsc_alloc_recv_skb()
drivers/net/hyperv/netvsc.c | 3 +-
drivers/net/hyperv/netvsc_bpf.c | 6 ++
drivers/net/hyperv/netvsc_drv.c | 18 +++-
drivers/net/hyperv/rndis_filter.c | 171 +++---
4 files changed, 136 insertions(+), 62 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..d9324961e0d64 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -887,6 +887,7 @@ static inline int netvsc_send_pkt(
int ret;
u32 ring_avail = hv_get_avail_to_write_percent(_channel->outbound);
+ memset(, 0, sizeof(struct nvsp_message));
nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT;
if (skb)
rpkt->channel_type = 0; /* 0 is RMC_DATA */
@@ -1306,7 +1307,7 @@ static void netvsc_send_table(struct net_device *ndev,
sizeof(union nvsp_6_message_uber);
/* Boundary check for all versions */
- if (offset > msglen - count * sizeof(u32)) {
+ if (msglen < count * sizeof(u32) || offset > msglen - count *
sizeof(u32)) {
netdev_err(ndev, "Received send-table offset too big:%u\n",
offset);
return;
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
index 440486d9c999e..11f0588a88843 100644
--- a/drivers/net/hyperv/netvsc_bpf.c
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -37,6 +37,12 @@ u32 netvsc_run_xdp(struct net_device *ndev, struct
netvsc_channel *nvchan,
if (!prog)
goto out;
+ /* Ensure that the below memcpy() won't overflow the page buffer. */
+ if (len > ndev->mtu + ETH_HLEN) {
+ act = XDP_DROP;
+ goto out;
+ }
+
/* allocate page buffer for data */
page = alloc_page(GFP_ATOMIC);
if (!page) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index f32f28311d573..e5501c1a0cbd4 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -760,6 +760,16 @@ void netvsc_linkstatus_callback(struct net_device *net,
if (indicate->status == RNDIS_STATUS_LINK_SPEED_CHANGE) {
u32 speed;
+ /* Validate status_buf_offset */
+ if (indicate->status_buflen < sizeof(speed) ||
+ indicate->status_buf_offset < sizeof(*indicate) ||
+ resp->msg_len - RNDIS_HEADER_SIZE <
indicate->status_buf_offset ||
+ resp->msg_len - RNDIS_HEADER_SIZE -
indicate->status_buf_offset
+ < indicate->status_buflen) {
+ netdev_err(net, "invalid rndis_indicate_status
packet\n");
+ return;
+ }
+
speed = *(u32 *)((void *)indicate
+ indicate->status_buf_offset) / 1;
ndev_ctx->speed = speed;
@@ -865,8 +875,14 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct
net_device *net,
*/
if (csum_info && csum_info->receive.ip_checksum_value_invalid &&
csum_info->receive.ip_checksum_succeeded &&
- skb->protocol == htons(ETH_P_IP))
+ skb->protocol == htons(ETH_P_IP)) {
+ /* Check that there is enough space to hold the IP header. */
+ if (skb_headlen(skb) < sizeof(struct iphdr)) {
+ kfree_skb(skb);
+ return NULL;
+ }
netvsc_comp_ipcsum(skb);
+ }
/* Do L4 checksum offload if enabled and present. */
if (csum_info && (net->features & NETIF_F_RXCSUM)) {
diff --git a/drivers/net/hyperv/rndis_filter.c
b/drivers/net/hyperv/rndis_filter.c
index 7e6dee2f02a43..68091a9a5070d 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -131,6
[PATCH] hv_netvsc: Add (more) validation for untrusted Hyper-V values
For additional robustness in the face of Hyper-V errors or malicious
behavior, validate all values that originate from packets that Hyper-V
has sent to the guest. Ensure that invalid values cannot cause indexing
off the end of an array, or subvert an existing validation via integer
overflow. Ensure that outgoing packets do not have any leftover guest
memory that has not been zeroed out.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Alexei Starovoitov
Cc: Daniel Borkmann
Cc: Andrii Nakryiko
Cc: Martin KaFai Lau
Cc: Song Liu
Cc: Yonghong Song
Cc: John Fastabend
Cc: KP Singh
Cc: net...@vger.kernel.org
Cc: b...@vger.kernel.org
---
drivers/net/hyperv/netvsc.c | 3 +-
drivers/net/hyperv/netvsc_bpf.c | 6 ++
drivers/net/hyperv/netvsc_drv.c | 16 ++-
drivers/net/hyperv/rndis_filter.c | 167 +++---
4 files changed, 130 insertions(+), 62 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 1510a236aa341..d9324961e0d64 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -887,6 +887,7 @@ static inline int netvsc_send_pkt(
int ret;
u32 ring_avail = hv_get_avail_to_write_percent(_channel->outbound);
+ memset(, 0, sizeof(struct nvsp_message));
nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT;
if (skb)
rpkt->channel_type = 0; /* 0 is RMC_DATA */
@@ -1306,7 +1307,7 @@ static void netvsc_send_table(struct net_device *ndev,
sizeof(union nvsp_6_message_uber);
/* Boundary check for all versions */
- if (offset > msglen - count * sizeof(u32)) {
+ if (msglen < count * sizeof(u32) || offset > msglen - count *
sizeof(u32)) {
netdev_err(ndev, "Received send-table offset too big:%u\n",
offset);
return;
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
index 440486d9c999e..11f0588a88843 100644
--- a/drivers/net/hyperv/netvsc_bpf.c
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -37,6 +37,12 @@ u32 netvsc_run_xdp(struct net_device *ndev, struct
netvsc_channel *nvchan,
if (!prog)
goto out;
+ /* Ensure that the below memcpy() won't overflow the page buffer. */
+ if (len > ndev->mtu + ETH_HLEN) {
+ act = XDP_DROP;
+ goto out;
+ }
+
/* allocate page buffer for data */
page = alloc_page(GFP_ATOMIC);
if (!page) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index f32f28311d573..ef3ad66d927a8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -760,6 +760,16 @@ void netvsc_linkstatus_callback(struct net_device *net,
if (indicate->status == RNDIS_STATUS_LINK_SPEED_CHANGE) {
u32 speed;
+ /* Validate status_buf_offset */
+ if (indicate->status_buflen < sizeof(speed) ||
+ indicate->status_buf_offset < sizeof(*indicate) ||
+ resp->msg_len - RNDIS_HEADER_SIZE <
indicate->status_buf_offset ||
+ resp->msg_len - RNDIS_HEADER_SIZE -
indicate->status_buf_offset
+ < indicate->status_buflen) {
+ netdev_err(net, "invalid rndis_indicate_status
packet\n");
+ return;
+ }
+
speed = *(u32 *)((void *)indicate
+ indicate->status_buf_offset) / 1;
ndev_ctx->speed = speed;
@@ -865,8 +875,12 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct
net_device *net,
*/
if (csum_info && csum_info->receive.ip_checksum_value_invalid &&
csum_info->receive.ip_checksum_succeeded &&
- skb->protocol == htons(ETH_P_IP))
+ skb->protocol == htons(ETH_P_IP)) {
+ /* Check that there is enough space to hold the IP header. */
+ if (skb_headlen(skb) < sizeof(struct iphdr))
+ return NULL;
netvsc_comp_ipcsum(skb);
+ }
/* Do L4 checksum offload if enabled and present. */
if (csum_info && (net->features & NETIF_F_RXCSUM)) {
diff --git a/drivers/net/hyperv/rndis_filter.c
b/drivers/net/hyperv/rndis_filter.c
index 7e6dee2f02a43..99c146f6a9d6f 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -131,66 +131,84 @@ static void dump_rndis_message(struct net_device *netdev,
{
switch (rndis_msg->ndis_msg_type) {
case RNDIS_MSG_PACKET:
- netdev_dbg(netdev, "RNDIS_MSG_PACKET (len %u, "
-
Re: [PATCH AUTOSEL 4.14 40/66] hv_netvsc: Validate number of allocated sub-channels
On Wed, Dec 23, 2020 at 02:47:56AM +, Michael Kelley wrote: > From: Sasha Levin Sent: Tuesday, December 22, 2020 6:22 PM > > > > From: "Andrea Parri (Microsoft)" > > > > [ Upstream commit 206ad34d52a2f1205c84d08c12fc116aad0eb407 ] > > > > Lack of validation could lead to out-of-bound reads and information > > leaks (cf. usage of nvdev->chan_table[]). Check that the number of > > allocated sub-channels fits into the expected range. > > > > Suggested-by: Saruhan Karademir > > Signed-off-by: Andrea Parri (Microsoft) > > Reviewed-by: Haiyang Zhang > > Acked-by: Jakub Kicinski > > Cc: "David S. Miller" > > Cc: Jakub Kicinski > > Cc: net...@vger.kernel.org > > Link: > > https://lore.kernel.org/linux-hyperv/20201118153310.112404-1-parri.and...@gmail.com/ > > Signed-off-by: Wei Liu > > Signed-off-by: Sasha Levin > > --- > > drivers/net/hyperv/rndis_filter.c | 5 + > > 1 file changed, 5 insertions(+) > > > > Sasha -- This patch is one of an ongoing group of patches where a Linux > guest running on Hyper-V will start assuming that hypervisor behavior might > be malicious, and guards against such behavior. Because this is a new > assumption, these patches are more properly treated as new functionality > rather than as bug fixes. So I would propose that we *not* bring such patches > back to stable branches. Thank you, Michael. Just to confirm, I agree with Michael's assessment above and I join his proposal to *not* backport such patches to stable. Thanks, Andrea
[PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()
Check that the packet is of the expected size at least, don't copy data
past the packet.
Reported-by: Saruhan Karademir
Signed-off-by: Andrea Parri (Microsoft)
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/storvsc_drv.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8714355cb63e7..4b8bde2750fac 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1250,6 +1250,12 @@ static void storvsc_on_channel_callback(void *context)
request = (struct storvsc_cmd_request *)
((unsigned long)desc->trans_id);
+ if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
+ stor_device->vmscsi_size_delta) {
+ dev_err(>device, "Invalid packet len\n");
+ continue;
+ }
+
if (request == _device->init_request ||
request == _device->reset_request) {
memcpy(>vstor_packet, packet,
--
2.25.1
[PATCH 2/3] scsi: storvsc: Resolve data race in storvsc_probe()
vmscsi_size_delta can be written concurrently by multiple instances of
storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS. Change the
global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
Suggested-by: Dexuan Cui
Signed-off-by: Andrea Parri (Microsoft)
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/storvsc_drv.c | 45 +-
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 64298aa2f151e..8714355cb63e7 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -216,18 +216,6 @@ struct vmscsi_request {
} __attribute((packed));
-
-/*
- * The size of the vmscsi_request has changed in win8. The
- * additional size is because of new elements added to the
- * structure. These elements are valid only when we are talking
- * to a win8 host.
- * Track the correction to size we need to apply. This value
- * will likely change during protocol negotiation but it is
- * valid to start by assuming pre-Win8.
- */
-static int vmscsi_size_delta = sizeof(struct vmscsi_win8_extension);
-
/*
* The list of storage protocols in order of preference.
*/
@@ -449,6 +437,17 @@ struct storvsc_device {
unsigned char path_id;
unsigned char target_id;
+ /*
+* The size of the vmscsi_request has changed in win8. The
+* additional size is because of new elements added to the
+* structure. These elements are valid only when we are talking
+* to a win8 host.
+* Track the correction to size we need to apply. This value
+* will likely change during protocol negotiation but it is
+* valid to start by assuming pre-Win8.
+*/
+ int vmscsi_size_delta;
+
/*
* Max I/O, the device can support.
*/
@@ -762,7 +761,7 @@ static void handle_multichannel_storage(struct hv_device
*device, int max_chns)
ret = vmbus_sendpacket(device->channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -816,9 +815,14 @@ static int storvsc_execute_vstor_op(struct hv_device
*device,
struct storvsc_cmd_request *request,
bool status_check)
{
+ struct storvsc_device *stor_device;
struct vstor_packet *vstor_packet;
int ret, t;
+ stor_device = get_out_stor_device(device);
+ if (!stor_device)
+ return -ENODEV;
+
vstor_packet = >vstor_packet;
init_completion(>wait_event);
@@ -826,7 +830,7 @@ static int storvsc_execute_vstor_op(struct hv_device
*device,
ret = vmbus_sendpacket(device->channel, vstor_packet,
(sizeof(struct vstor_packet) -
- vmscsi_size_delta),
+ stor_device->vmscsi_size_delta),
(unsigned long)request,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -903,7 +907,7 @@ static int storvsc_channel_init(struct hv_device *device,
bool is_fc)
sense_buffer_size =
vmstor_protocols[i].sense_buffer_size;
- vmscsi_size_delta =
+ stor_device->vmscsi_size_delta =
vmstor_protocols[i].vmscsi_size_delta;
break;
@@ -1249,7 +1253,7 @@ static void storvsc_on_channel_callback(void *context)
if (request == _device->init_request ||
request == _device->reset_request) {
memcpy(>vstor_packet, packet,
- (sizeof(struct vstor_packet) -
vmscsi_size_delta));
+ (sizeof(struct vstor_packet) -
stor_device->vmscsi_size_delta));
complete(>wait_event);
} else {
storvsc_on_receive(stor_device, packet, request);
@@ -1461,7 +1465,7 @@ static int storvsc_do_io(struct hv_device *device,
vstor_packet->flags |= REQUEST_COMPLETION_FLAG;
vstor_packet->vm_srb.length = (sizeof(struct vmscsi_request) -
- vmscsi_size_delta);
+ stor_device->vmscsi_size_delta);
vstor_packet->vm_srb.sense_info_length = sense_buffer_size;
@@ -1
[PATCH 1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
Current code overestimates the value of max_outstanding_req_per_channel for Win8 and newer hosts, since vmscsi_size_delta is set to the initial value of sizeof(vmscsi_win8_extension) rather than zero. This may lead to wrong decisions when using ring_avail_percent_lowater equals to zero. The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and older hosts. A better choice, keeping the algorithm for the estimation simple, is to err the other way around, i.e., to underestimate for Win7 and older but to use the exact value for Win8 and newer. Suggested-by: Dexuan Cui Signed-off-by: Andrea Parri (Microsoft) Cc: "James E.J. Bottomley" Cc: "Martin K. Petersen" Cc: linux-s...@vger.kernel.org --- drivers/scsi/storvsc_drv.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index ded00a89bfc4e..64298aa2f151e 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -2141,12 +2141,15 @@ static int __init storvsc_drv_init(void) * than the ring buffer size since that page is reserved for * the ring buffer indices) by the max request size (which is * vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64) +* +* The computation underestimates max_outstanding_req_per_channel +* for Win7 and older hosts because it does not take into account +* the vmscsi_size_delta correction to the max request size. */ max_outstanding_req_per_channel = ((storvsc_ringbuffer_size - PAGE_SIZE) / ALIGN(MAX_MULTIPAGE_BUFFER_PACKET + - sizeof(struct vstor_packet) + sizeof(u64) - - vmscsi_size_delta, + sizeof(struct vstor_packet) + sizeof(u64), sizeof(u64))); #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS) -- 2.25.1
[PATCH 0/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback() -- Take 2
Hi all,
This series is to address the problems mentioned in:
4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in
storvsc_on_channel_callback()"")
(cf., in particular, patch 2/3) and to re-introduce the validation in
question (patch 3/3); patch 1/3 emerged from internal review of these
two patches and is a related fix.
Thanks,
Andrea
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
Andrea Parri (Microsoft) (3):
scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
scsi: storvsc: Resolve data race in storvsc_probe()
scsi: storvsc: Validate length of incoming packet in
storvsc_on_channel_callback()
drivers/scsi/storvsc_drv.c | 58 +++---
1 file changed, 36 insertions(+), 22 deletions(-)
--
2.25.1
Re: [PATCH AUTOSEL 5.9 15/23] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()
Hi Sasha,
On Sat, Dec 12, 2020 at 11:07:56AM -0500, Sasha Levin wrote:
> From: "Andrea Parri (Microsoft)"
>
> [ Upstream commit 3b8c72d076c42bf27284cda7b2b2b522810686f8 ]
FYI, we found that this commit introduced a regression and posted a
revert:
https://lkml.kernel.org/r/20201211131404.21359-1-parri.and...@gmail.com
Same comment for the AUTOSEL 5.4, 4.19 and 4.14 you've just posted.
Andrea
>
> Check that the packet is of the expected size at least, don't copy data
> past the packet.
>
> Link: https://lore.kernel.org/r/20201118145348.109879-1-parri.and...@gmail.com
> Cc: "James E.J. Bottomley"
> Cc: "Martin K. Petersen"
> Cc: linux-s...@vger.kernel.org
> Reported-by: Saruhan Karademir
> Signed-off-by: Andrea Parri (Microsoft)
> Signed-off-by: Martin K. Petersen
> Signed-off-by: Sasha Levin
> ---
> drivers/scsi/storvsc_drv.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8f5f5dc863a4a..6779ee4edfee3 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1246,6 +1246,11 @@ static void storvsc_on_channel_callback(void *context)
> request = (struct storvsc_cmd_request *)
> ((unsigned long)desc->trans_id);
>
> + if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
> vmscsi_size_delta) {
> + dev_err(>device, "Invalid packet len\n");
> + continue;
> + }
> +
> if (request == _device->init_request ||
> request == _device->reset_request) {
> memcpy(>vstor_packet, packet,
> --
> 2.27.0
>
[PATCH] Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"
This reverts commit 3b8c72d076c42bf27284cda7b2b2b522810686f8.
Dexuan reported a regression where StorVSC fails to probe a device (and
where, consequently, the VM may fail to boot). The root-cause analysis
led to a long-standing race condition that is exposed by the validation
/commit in question. Let's put the new validation aside until a proper
solution for that race condition is in place.
Signed-off-by: Andrea Parri (Microsoft)
Cc: Dexuan Cui
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-s...@vger.kernel.org
---
drivers/scsi/storvsc_drv.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 99c8ff81de746..ded00a89bfc4e 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1246,11 +1246,6 @@ static void storvsc_on_channel_callback(void *context)
request = (struct storvsc_cmd_request *)
((unsigned long)desc->trans_id);
- if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
vmscsi_size_delta) {
- dev_err(>device, "Invalid packet len\n");
- continue;
- }
-
if (request == _device->init_request ||
request == _device->reset_request) {
memcpy(>vstor_packet, packet,
--
2.25.1
[PATCH v3 3/6] Drivers: hv: vmbus: Copy the hv_message in vmbus_on_msg_dpc()
Since the message is in memory shared with the host, an erroneous or a
malicious Hyper-V could 'corrupt' the message while vmbus_on_msg_dpc()
or individual message handlers are executing. To prevent it, copy the
message into private memory.
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since v2:
- Revisit commit message and inline comment
drivers/hv/vmbus_drv.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 44bcf9ccdaf5f..b1c5a89d75f9d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1054,14 +1054,14 @@ void vmbus_on_msg_dpc(unsigned long data)
{
struct hv_per_cpu_context *hv_cpu = (void *)data;
void *page_addr = hv_cpu->synic_message_page;
- struct hv_message *msg = (struct hv_message *)page_addr +
+ struct hv_message msg_copy, *msg = (struct hv_message *)page_addr +
VMBUS_MESSAGE_SINT;
struct vmbus_channel_message_header *hdr;
enum vmbus_channel_message_type msgtype;
const struct vmbus_channel_message_table_entry *entry;
struct onmessage_work_context *ctx;
- u32 message_type = msg->header.message_type;
__u8 payload_size;
+ u32 message_type;
/*
* 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
@@ -1070,11 +1070,20 @@ void vmbus_on_msg_dpc(unsigned long data)
*/
BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
+ /*
+* Since the message is in memory shared with the host, an erroneous or
+* malicious Hyper-V could modify the message while vmbus_on_msg_dpc()
+* or individual message handlers are executing; to prevent this, copy
+* the message into private memory.
+*/
+ memcpy(_copy, msg, sizeof(struct hv_message));
+
+ message_type = msg_copy.header.message_type;
if (message_type == HVMSG_NONE)
/* no msg */
return;
- hdr = (struct vmbus_channel_message_header *)msg->u.payload;
+ hdr = (struct vmbus_channel_message_header *)msg_copy.u.payload;
msgtype = hdr->msgtype;
trace_vmbus_on_msg_dpc(hdr);
@@ -1084,7 +1093,7 @@ void vmbus_on_msg_dpc(unsigned long data)
goto msg_handled;
}
- payload_size = msg->header.payload_size;
+ payload_size = msg_copy.header.payload_size;
if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
WARN_ONCE(1, "payload size is too large (%d)\n", payload_size);
goto msg_handled;
@@ -1106,7 +1115,7 @@ void vmbus_on_msg_dpc(unsigned long data)
return;
INIT_WORK(>work, vmbus_onmessage_work);
- memcpy(>msg, msg, sizeof(msg->header) + payload_size);
+ memcpy(>msg, _copy, sizeof(msg->header) +
payload_size);
/*
* The host can generate a rescind message while we
--
2.25.1
[PATCH v3 1/6] Drivers: hv: vmbus: Initialize memory to be sent to the host
__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
Signed-off-by: Andrea Parri (Microsoft)
Reviewed-by: Michael Kelley
---
Changes since v2:
- Add Reviewed-by: tag
drivers/hv/channel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0d63862d65518..9aa789e5f22bb 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -621,7 +621,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
goto error_clean_ring;
/* Create and init the channel open message */
- open_info = kmalloc(sizeof(*open_info) +
+ open_info = kzalloc(sizeof(*open_info) +
sizeof(struct vmbus_channel_open_channel),
GFP_KERNEL);
if (!open_info) {
@@ -748,7 +748,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32
gpadl_handle)
unsigned long flags;
int ret;
- info = kmalloc(sizeof(*info) +
+ info = kzalloc(sizeof(*info) +
sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
if (!info)
return -ENOMEM;
--
2.25.1
[PATCH v3 5/6] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
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
Signed-off-by: Andrea Parri (Microsoft)
---
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(_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
+* _connection.channel_mutex.
+*/
+ if (channel->rescind_ref) {
+ mutex_unlock(_connection.channel_mutex);
+ return;
+ }
+ channel->rescind_ref = true;
+ }
mutex_unlock(_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 v3 6/6] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[]
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
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since v2:
- Release channel_mutex before 'return' in vmbus_onoffer() error path
drivers/hv/channel_mgmt.c | 40 +--
drivers/hv/hyperv_vmbus.h | 2 +-
2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 68950a1e4b638..2c15693b86f1e 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -354,10 +354,12 @@ static void free_channel(struct vmbus_channel *channel)
kobject_put(>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(_connection.offer_in_progress);
+ if (vmbus_channel_map_relid(newchannel)) {
+ mutex_unlock(_connection.channel_mutex);
+ kfree(newchannel);
+ return;
+ }
+
list_for_each_entry(channel, _connection.chn_list, listentry) {
if (guid_equal(>offermsg.offer.if_type,
>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(_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(>sc_list, >sc_list);
}
- vmbus_channel_map_relid(newchannel);
-
mutex_unlock(_connection.channel_mutex);
cpus_read_unlock();
@@ -920,6 +926,8 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
+ u32 relid = offer->child_relid;
+
/*
* We're resuming from hibernation: all the sub-channel and
* hv_sock channels we had before the hibernation should have
@@ -954,8 +962,12 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
atomic_dec(_connection.offer_in_progress);
WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
+ if (WARN_ON(vmbus_connection.channels[relid] != NULL)) {
+ mutex_unlock(_connection.channel_mutex);
+ return;
+ }
/* Fix up the relid. */
- oldchannel->offermsg.child_relid = offer->child_relid;
+ oldchannel->offermsg.child_relid = relid;
offer_sz = sizeof(*offer);
if (memcmp(offer, >offermsg, offer_sz) != 0) {
@@ -967,7 +979,7 @@ static void vmbus_onoffer(struct
vmbus_cha
[PATCH v3 2/6] Drivers: hv: vmbus: Reduce number of references to message in vmbus_on_msg_dpc()
Simplify the function by removing various references to the hv_message
'msg', introduce local variables 'msgtype' and 'payload_size'.
Suggested-by: Juan Vazquez
Suggested-by: Michael Kelley
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since v2:
- Squash patches #2 and #3
- Revisit commit message
drivers/hv/vmbus_drv.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 502f8cd95f6d4..44bcf9ccdaf5f 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1057,9 +1057,11 @@ void vmbus_on_msg_dpc(unsigned long data)
struct hv_message *msg = (struct hv_message *)page_addr +
VMBUS_MESSAGE_SINT;
struct vmbus_channel_message_header *hdr;
+ enum vmbus_channel_message_type msgtype;
const struct vmbus_channel_message_table_entry *entry;
struct onmessage_work_context *ctx;
u32 message_type = msg->header.message_type;
+ __u8 payload_size;
/*
* 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
@@ -1073,40 +1075,38 @@ void vmbus_on_msg_dpc(unsigned long data)
return;
hdr = (struct vmbus_channel_message_header *)msg->u.payload;
+ msgtype = hdr->msgtype;
trace_vmbus_on_msg_dpc(hdr);
- if (hdr->msgtype >= CHANNELMSG_COUNT) {
- WARN_ONCE(1, "unknown msgtype=%d\n", hdr->msgtype);
+ if (msgtype >= CHANNELMSG_COUNT) {
+ WARN_ONCE(1, "unknown msgtype=%d\n", msgtype);
goto msg_handled;
}
- if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
- WARN_ONCE(1, "payload size is too large (%d)\n",
- msg->header.payload_size);
+ payload_size = msg->header.payload_size;
+ if (payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
+ WARN_ONCE(1, "payload size is too large (%d)\n", payload_size);
goto msg_handled;
}
- entry = _message_table[hdr->msgtype];
+ entry = _message_table[msgtype];
if (!entry->message_handler)
goto msg_handled;
- if (msg->header.payload_size < entry->min_payload_len) {
- WARN_ONCE(1, "message too short: msgtype=%d len=%d\n",
- hdr->msgtype, msg->header.payload_size);
+ if (payload_size < entry->min_payload_len) {
+ WARN_ONCE(1, "message too short: msgtype=%d len=%d\n", msgtype,
payload_size);
goto msg_handled;
}
if (entry->handler_type == VMHT_BLOCKING) {
- ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
- GFP_ATOMIC);
+ ctx = kmalloc(sizeof(*ctx) + payload_size, GFP_ATOMIC);
if (ctx == NULL)
return;
INIT_WORK(>work, vmbus_onmessage_work);
- memcpy(>msg, msg, sizeof(msg->header) +
- msg->header.payload_size);
+ memcpy(>msg, msg, sizeof(msg->header) + payload_size);
/*
* The host can generate a rescind message while we
@@ -1115,7 +1115,7 @@ void vmbus_on_msg_dpc(unsigned long data)
* by offer_in_progress and by channel_mutex. See also the
* inline comments in vmbus_onoffer_rescind().
*/
- switch (hdr->msgtype) {
+ switch (msgtype) {
case CHANNELMSG_RESCIND_CHANNELOFFER:
/*
* If we are handling the rescind message;
--
2.25.1
[PATCH v3 0/6] Drivers: hv: vmbus: More VMBus-hardening changes
Integrating feedback from Juan, Michael and Wei. [1] Changelogs are inline/in the patches. Thanks, Andrea [1] https://lkml.kernel.org/r/20201202092214.13520-1-parri.and...@gmail.com Andrea Parri (Microsoft) (6): Drivers: hv: vmbus: Initialize memory to be sent to the host Drivers: hv: vmbus: Reduce number of references to message in vmbus_on_msg_dpc() Drivers: hv: vmbus: Copy the hv_message in vmbus_on_msg_dpc() Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind() Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[] drivers/hv/channel.c | 4 +-- drivers/hv/channel_mgmt.c | 55 +++ drivers/hv/hyperv_vmbus.h | 2 +- drivers/hv/vmbus_drv.c| 43 ++ include/linux/hyperv.h| 1 + 5 files changed, 69 insertions(+), 36 deletions(-) -- 2.25.1
[PATCH v3 4/6] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()
When channel->device_obj is non-NULL, vmbus_onoffer_rescind() could
invoke put_device(), that will eventually release the device and free
the channel object (cf. vmbus_device_release()). However, a pointer
to the object is dereferenced again later to load the primary_channel.
The use-after-free can be avoided by noticing that this load/check is
redundant if device_obj is non-NULL: primary_channel must be NULL if
device_obj is non-NULL, cf. vmbus_add_channel_work().
Fixes: 54a66265d6754b ("Drivers: hv: vmbus: Fix rescind handling")
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
Reviewed-by: Michael Kelley
---
Changes since v2:
- Add Reviewed-by: tag
drivers/hv/channel_mgmt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 5bc5eef5da159..4072fd1f22146 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1116,8 +1116,7 @@ static void vmbus_onoffer_rescind(struct
vmbus_channel_message_header *hdr)
vmbus_device_unregister(channel->device_obj);
put_device(dev);
}
- }
- if (channel->primary_channel != NULL) {
+ } else if (channel->primary_channel != NULL) {
/*
* Sub-channel is being rescinded. Following is the channel
* close sequence when initiated from the driveri (refer to
--
2.25.1
[PATCH v3] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer
From: Andres Beltran
Pointers to ring-buffer packets sent by Hyper-V are used within the
guest VM. Hyper-V can send packets with erroneous values or modify
packet fields after they are processed by the guest. To defend
against these scenarios, return a copy of the incoming VMBus packet
after validating its length and offset fields in hv_pkt_iter_first().
In this way, the packet can no longer be modified by the host.
Signed-off-by: Andres Beltran
Co-developed-by: Andrea Parri (Microsoft)
Signed-off-by: Andrea Parri (Microsoft)
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: net...@vger.kernel.org
Cc: linux-s...@vger.kernel.org
---
drivers/hv/channel.c | 9 ++--
drivers/hv/hv_fcopy.c | 1 +
drivers/hv/hv_kvp.c | 1 +
drivers/hv/hyperv_vmbus.h | 2 +-
drivers/hv/ring_buffer.c | 82 ++-
drivers/net/hyperv/hyperv_net.h | 3 ++
drivers/net/hyperv/netvsc.c | 2 +
drivers/net/hyperv/rndis_filter.c | 2 +
drivers/scsi/storvsc_drv.c| 10
include/linux/hyperv.h| 48 +++---
net/vmw_vsock/hyperv_transport.c | 4 +-
11 files changed, 139 insertions(+), 25 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 6fb0c76bfbf81..0d63862d65518 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -597,12 +597,15 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
newchannel->onchannel_callback = onchannelcallback;
newchannel->channel_callback_context = context;
- err = hv_ringbuffer_init(>outbound, page, send_pages);
+ if (!newchannel->max_pkt_size)
+ newchannel->max_pkt_size = VMBUS_DEFAULT_MAX_PKT_SIZE;
+
+ err = hv_ringbuffer_init(>outbound, page, send_pages, 0);
if (err)
goto error_clean_ring;
- err = hv_ringbuffer_init(>inbound,
-[send_pages], recv_pages);
+ err = hv_ringbuffer_init(>inbound, [send_pages],
+recv_pages, newchannel->max_pkt_size);
if (err)
goto error_clean_ring;
diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
index 59ce85e00a028..660036da74495 100644
--- a/drivers/hv/hv_fcopy.c
+++ b/drivers/hv/hv_fcopy.c
@@ -349,6 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
{
recv_buffer = srv->recv_buffer;
fcopy_transaction.recv_channel = srv->channel;
+ fcopy_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
/*
* When this driver loads, the user level daemon that
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index b49962d312cef..c698592b83e42 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -757,6 +757,7 @@ hv_kvp_init(struct hv_util_service *srv)
{
recv_buffer = srv->recv_buffer;
kvp_transaction.recv_channel = srv->channel;
+ kvp_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 4;
/*
* When this driver loads, the user level daemon that
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 9416e09ebd58c..42f3d9d123a12 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -174,7 +174,7 @@ extern int hv_synic_cleanup(unsigned int cpu);
void hv_ringbuffer_pre_init(struct vmbus_channel *channel);
int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
- struct page *pages, u32 pagecnt);
+ struct page *pages, u32 pagecnt, u32 max_pkt_size);
void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 35833d4d1a1dc..29e90477363a8 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -190,7 +190,7 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)
/* Initialize the ring buffer. */
int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
- struct page *pages, u32 page_cnt)
+ struct page *pages, u32 page_cnt, u32 max_pkt_size)
{
int i;
struct page **pages_wraparound;
@@ -232,6 +232,14 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info
*ring_info,
sizeof(struct hv_ring_buffer);
ring_info->priv_read_index = 0;
+ /* Initialize buffer that holds copies of incoming packets */
+ if (max_pkt_size) {
+ ring_info->pkt_buffer = kzalloc(max_pkt_size, GFP_KERNEL);
+ if (!ring_info->pkt_buffer)
+ return -ENOMEM;
+ ring_info->pkt_buffer_size = max_pkt_size;
+ }
+
spin_lock_init(_info->ring_lock);
return 0;
@@ -244,6 +252,9 @@ void hv_ringbuffer_cleanup(struct hv_ring_buffer_info
*ring_info)
v
Re: [PATCH v2] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer
> > @@ -419,17 +446,52 @@ static u32 hv_pkt_iter_avail(const struct
> > hv_ring_buffer_info *rbi)
> > struct vmpacket_descriptor *hv_pkt_iter_first(struct vmbus_channel
> > *channel)
> > {
> > struct hv_ring_buffer_info *rbi = >inbound;
> > - struct vmpacket_descriptor *desc;
> > + struct vmpacket_descriptor *desc, *desc_copy;
> > + u32 bytes_avail, pkt_len, pkt_offset;
> >
> > - hv_debug_delay_test(channel, MESSAGE_DELAY);
> > - if (hv_pkt_iter_avail(rbi) < sizeof(struct vmpacket_descriptor))
> > + desc = hv_pkt_iter_first_raw(channel);
> > + if (!desc)
> > return NULL;
> >
> > - desc = hv_get_ring_buffer(rbi) + rbi->priv_read_index;
> > - if (desc)
> > - prefetch((char *)desc + (desc->len8 << 3));
> > + bytes_avail = hv_pkt_iter_avail(rbi);
> > +
> > + /*
> > +* Ensure the compiler does not use references to incoming Hyper-V
> > values (which
> > +* could change at any moment) when reading local variables later in
> > the code
> > +*/
> > + pkt_len = READ_ONCE(desc->len8) << 3;
> > + pkt_offset = READ_ONCE(desc->offset8) << 3;
> > +
> > + /*
> > +* If pkt_len is invalid, set it to the smaller of hv_pkt_iter_avail()
> > and
> > +* rbi->pkt_buffer_size
> > +*/
> > + if (rbi->pkt_buffer_size < bytes_avail)
> > + bytes_avail = rbi->pkt_buffer_size;
>
> I think the above could be combined with the earlier call to
> hv_pkt_iter_avail(),
> and more logically expressed as:
>
> bytes_avail = min(rbi->pkt_buffer_size, hv_pkt_iter_avail(rbi));
>
>
> This is a minor nit. Everything else in this patch looks good to me.
Thanks for the feedback, Michael; I'll send v3 to address it shortly.
Andrea
Re: [PATCH v2 4/7] Drivers: hv: vmbus: Copy the hv_message object in vmbus_on_msg_dpc()
On Sun, Dec 06, 2020 at 06:39:39PM +, Michael Kelley wrote: > From: Andrea Parri (Microsoft) Sent: Wednesday, > December 2, 2020 1:22 AM > > > > The hv_message object is in memory shared with the host. To prevent > > an erroneous or a malicious host from 'corrupting' such object, copy > > the object into private memory. [...] > But if we're going to just make a copy at the start and use the copy for > everything, then the motivation for the changes in Patches 2 and 3 goes > away. The double-fetch problem is solved entirely by Patch 4. The > changes in Patches 2 and 3 *are* nice for simplifying the code, but > that's all. The code simplification is still useful as prep to reduce the > number of references to "msg" that have to be changed to "msg_copy", > but the commit message should be changed to reflect that, rather than > to eliminate double fetches. I'm okay with this: I will revisit the commit messages of 2/3, maybe squash those 'simplifications' into a single patch, and then 'solve' /guard against the double fetch problem with 4. Thanks, Andrea
Re: [PATCH 3/6] Drivers: hv: vmbus: Avoid double fetch of payload_size in vmbus_on_msg_dpc()
On Sun, Dec 06, 2020 at 05:14:18PM +, Michael Kelley wrote: > From: Andrea Parri (Microsoft) 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
Re: [PATCH 2/6] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()
On Sun, Dec 06, 2020 at 05:10:26PM +, Michael Kelley wrote: > From: Andrea Parri (Microsoft) 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
Re: [PATCH 1/6] Drivers: hv: vmbus: Initialize memory to be sent to the host
On Sun, Dec 06, 2020 at 04:59:32PM +, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) 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
> > Signed-off-by: Andrea Parri (Microsoft)
> > ---
> > 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
Thank you for the review.
Please notice that I posted a v2 of this series:
https://lkml.kernel.org/r/20201202092214.13520-1-parri.and...@gmail.com
Thanks,
Andrea
Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()
On Wed, Dec 02, 2020 at 01:40:04PM +, Wei Liu wrote: > On Wed, Dec 02, 2020 at 02:37:16PM +0100, Andrea Parri wrote: > > > > @@ -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; > > > > > > Should READ_ONCE be used here? > > > > I think it should. Thank you for pointing this out. > > Glad I can help. > > The same comment applies to other patches as well, of course. (As discussed offline/for reference:) I can spot a similar case in patch #3; however, #4 is supposed to make that access 'non-shared'. I should probably just squash patches #3 and #4; I'll try to do so in v3... Thanks, Andrea
Re: [PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()
> > @@ -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; > > Should READ_ONCE be used here? I think it should. Thank you for pointing this out. Andrea
[PATCH v2 5/7] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()
When channel->device_obj is non-NULL, vmbus_onoffer_rescind() could
invoke put_device(), that will eventually release the device and free
the channel object (cf. vmbus_device_release()). However, a pointer
to the object is dereferenced again later to load the primary_channel.
The use-after-free can be avoided by noticing that this load/check is
redundant if device_obj is non-NULL: primary_channel must be NULL if
device_obj is non-NULL, cf. vmbus_add_channel_work().
Fixes: 54a66265d6754b ("Drivers: hv: vmbus: Fix rescind handling")
Reported-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
---
Changes since v1:
- Fix typo in commit message
- Add Fixes: tag
drivers/hv/channel_mgmt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 5bc5eef5da159..4072fd1f22146 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1116,8 +1116,7 @@ static void vmbus_onoffer_rescind(struct
vmbus_channel_message_header *hdr)
vmbus_device_unregister(channel->device_obj);
put_device(dev);
}
- }
- if (channel->primary_channel != NULL) {
+ } else if (channel->primary_channel != NULL) {
/*
* Sub-channel is being rescinded. Following is the channel
* close sequence when initiated from the driveri (refer to
--
2.25.1
[PATCH v2 6/7] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind()
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
Signed-off-by: Andrea Parri (Microsoft)
---
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(_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
+* _connection.channel_mutex.
+*/
+ if (channel->rescind_ref) {
+ mutex_unlock(_connection.channel_mutex);
+ return;
+ }
+ channel->rescind_ref = true;
+ }
mutex_unlock(_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 v2 3/7] Drivers: hv: vmbus: Avoid double fetch of payload_size in vmbus_on_msg_dpc()
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
Signed-off-by: Andrea Parri (Microsoft)
---
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(>work, vmbus_onmessage_work);
- memcpy(>msg, msg, sizeof(msg->header) +
- msg->header.payload_size);
+ memcpy(>msg, msg, sizeof(msg->header) + payload_size);
/*
* The host can generate a rescind message while we
--
2.25.1
[PATCH v2 4/7] Drivers: hv: vmbus: Copy the hv_message object in vmbus_on_msg_dpc()
The hv_message object is in memory shared with the host. To prevent
an erroneous or a malicious host from 'corrupting' such object, copy
the object into private memory.
Suggested-by: Juan Vazquez
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/vmbus_drv.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0e39f1d6182e9..796202aa7f9b4 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1054,14 +1054,24 @@ void vmbus_on_msg_dpc(unsigned long data)
{
struct hv_per_cpu_context *hv_cpu = (void *)data;
void *page_addr = hv_cpu->synic_message_page;
- struct hv_message *msg = (struct hv_message *)page_addr +
+ struct hv_message msg_copy, *msg = (struct hv_message *)page_addr +
VMBUS_MESSAGE_SINT;
- __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;
struct onmessage_work_context *ctx;
- u32 message_type = msg->header.message_type;
+ __u8 payload_size;
+ u32 message_type;
+
+ /*
+* The hv_message object is in memory shared with the host. Prevent an
+* erroneous or malicious host from 'corrupting' such object by copying
+* the object to private memory.
+*/
+ memcpy(_copy, msg, sizeof(struct hv_message));
+
+ payload_size = msg_copy.header.payload_size;
+ message_type = msg_copy.header.message_type;
/*
* 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
@@ -1074,13 +1084,7 @@ 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;
+ hdr = (struct vmbus_channel_message_header *)msg_copy.u.payload;
msgtype = hdr->msgtype;
trace_vmbus_on_msg_dpc(hdr);
@@ -,7 +1115,7 @@ void vmbus_on_msg_dpc(unsigned long data)
return;
INIT_WORK(>work, vmbus_onmessage_work);
- memcpy(>msg, msg, sizeof(msg->header) + payload_size);
+ memcpy(>msg, _copy, sizeof(msg_copy.header) +
payload_size);
/*
* The host can generate a rescind message while we
--
2.25.1
[PATCH v2 7/7] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[]
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
Signed-off-by: Andrea Parri (Microsoft)
---
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(>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(_connection.offer_in_progress);
+ if (vmbus_channel_map_relid(newchannel)) {
+ mutex_unlock(_connection.channel_mutex);
+ kfree(newchannel);
+ return;
+ }
+
list_for_each_entry(channel, _connection.chn_list, listentry) {
if (guid_equal(>offermsg.offer.if_type,
>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(_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(>sc_list, >sc_list);
}
- vmbus_channel_map_relid(newchannel);
-
mutex_unlock(_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(_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, >offermsg, offer_sz) != 0) {
@@ -967,7 +977,7 @@ static void vmbus_onoffer(struct
vmbus_channel_message_header *hdr)
* reoffers the device upon resume.
[PATCH v2 0/7] Drivers: hv: vmbus: More VMBus-hardening changes
Hi all, This is v2 of [1], integrating feedback from Juan and Wei and adding patch 4/7 (after Juan's suggestion). Changelogs are in the patches. Thanks, Andrea [1] https://lkml.kernel.org/r/20201118143649.108465-1-parri.and...@gmail.com Andrea Parri (Microsoft) (7): 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: Copy the hv_message object 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 | 53 +++ drivers/hv/hyperv_vmbus.h | 2 +- drivers/hv/vmbus_drv.c| 43 ++- include/linux/hyperv.h| 1 + 5 files changed, 67 insertions(+), 36 deletions(-) -- 2.25.1
[PATCH v2 1/7] Drivers: hv: vmbus: Initialize memory to be sent to the host
__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
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 0d63862d65518..9aa789e5f22bb 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -621,7 +621,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
goto error_clean_ring;
/* Create and init the channel open message */
- open_info = kmalloc(sizeof(*open_info) +
+ open_info = kzalloc(sizeof(*open_info) +
sizeof(struct vmbus_channel_open_channel),
GFP_KERNEL);
if (!open_info) {
@@ -748,7 +748,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32
gpadl_handle)
unsigned long flags;
int ret;
- info = kmalloc(sizeof(*info) +
+ info = kzalloc(sizeof(*info) +
sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
if (!info)
return -ENOMEM;
--
2.25.1
[PATCH v2 2/7] Drivers: hv: vmbus: Avoid double fetch of msgtype in vmbus_on_msg_dpc()
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
Signed-off-by: Andrea Parri (Microsoft)
---
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 = _message_table[hdr->msgtype];
+ entry = _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] Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE message type
On Sun, Nov 29, 2020 at 06:29:55PM +, Michael Kelley wrote:
> From: Andrea Parri (Microsoft) Sent: Thursday,
> November 26, 2020 11:12 AM
> >
> > Quoting from commit 7527810573436f ("Drivers: hv: vmbus: Introduce
> > the CHANNELMSG_MODIFYCHANNEL message type"),
> >
> > "[...] Hyper-V can *not* currently ACK CHANNELMSG_MODIFYCHANNEL
> >messages with the promise that (after the ACK is sent) the
> >channel won't send any more interrupts to the "old" CPU.
> >
> >The peculiarity of the CHANNELMSG_MODIFYCHANNEL messages is
> >problematic if the user want to take a CPU offline, since we
> >don't want to take a CPU offline (and, potentially, "lose"
> >channel interrupts on such CPU) if the host is still processing
> >a CHANNELMSG_MODIFYCHANNEL message associated to that CPU."
> >
> > Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE(24) message type,
> > which embodies the type of the CHANNELMSG_MODIFYCHANNEL ACK.
>
> I feel like the commit message needs a bit more detail about the new
> functionality that is added, including introducing the new VMbus protocol
> version 5.3, and then waiting for the response message when running
> with that protocol version of later. Also, when taking a CPU offline, new
> functionality ensures that there are no pending channel interrupts for
> that CPU.
I'll add details along these lines in the next submission.
>
> Could/should this patch be broken into multiple patches?
>
> 1) Introduce and negotiate VMbus protocol version 5.3. Note that just
> negotiating version 5.3 should be safe because any ACK messages will
> be cleanly ignored by the old code.
> 2) Check for pending channel interrupts before taking a CPU offline.
> Wouldn't this check be valid even with the existing code where there is no
> ACK message? The old code is, in effect, assuming that enough time has
> passed such that the modify channel message has been processed.
> 3) Code to wait for an ACK message, and code to receive and process an
> ACK message.
I think that the patch could be broken into multiple patches. I'm still
wondering whether we could/should invoke hv_synic_event_pending() without
an ACK message (like the description above seems to suggest), say, from
the introduction of MODIFYCHANNEL messages... Thoughts?
> > +static int send_modifychannel_with_ack(struct vmbus_channel *channel, u32
> > target_vp)
> > +{
> > + struct vmbus_channel_modifychannel *msg;
> > + struct vmbus_channel_msginfo *info;
> > + unsigned long flags;
> > + int ret;
> > +
> > + info = kzalloc(sizeof(struct vmbus_channel_msginfo) +
> > + sizeof(struct vmbus_channel_modifychannel),
> > + GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > +
> > + init_completion(>waitevent);
> > + info->waiting_channel = channel;
> > +
> > + msg = (struct vmbus_channel_modifychannel *)info->msg;
> > + msg->header.msgtype = CHANNELMSG_MODIFYCHANNEL;
> > + msg->child_relid = channel->offermsg.child_relid;
> > + msg->target_vp = target_vp;
> > +
> > + spin_lock_irqsave(_connection.channelmsg_lock, flags);
> > + list_add_tail(>msglistentry, _connection.chn_msg_list);
> > + spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
> > +
> > + if (channel->rescind) {
> > + ret = -ENODEV;
> > + goto free_info;
> > + }
>
> Does the above check really do anything? Such checks are sprinkled throughout
> the VMbus code, and I've always questioned their efficacy. The rescind flag
> can
> be set without holding the channel_mutex, so as soon as the above code tests
> the flag, it seems like it could change.
Same question (and questioning) here actually. I'm planning to remove
this check in v2. Similarly for the ->rescind check below.
>
> > +
> > + ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_modifychannel),
> > true);
>
> Use "sizeof(*msg)" instead?
Applied.
>
> > + trace_vmbus_send_modifychannel(msg, ret);
> > + if (ret != 0)
> > + goto clean_msglist;
> > +
> > + /*
> > +* Release channel_mutex; otherwise, vmbus_onoffer_rescind() could
> > block on
> > +* the mutex and be unable to signal the completion.
> > +*/
> > + mutex_unlock(_connection.channel_mutex);
> > + wait_for_completion(>waitevent);
> > + mutex_lock(_connection.channel_mutex);
>
> The calling function, target_cpu_store(), obtai
[PATCH] Drivers: hv: vmbus: Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE message type
Quoting from commit 7527810573436f ("Drivers: hv: vmbus: Introduce
the CHANNELMSG_MODIFYCHANNEL message type"),
"[...] Hyper-V can *not* currently ACK CHANNELMSG_MODIFYCHANNEL
messages with the promise that (after the ACK is sent) the
channel won't send any more interrupts to the "old" CPU.
The peculiarity of the CHANNELMSG_MODIFYCHANNEL messages is
problematic if the user want to take a CPU offline, since we
don't want to take a CPU offline (and, potentially, "lose"
channel interrupts on such CPU) if the host is still processing
a CHANNELMSG_MODIFYCHANNEL message associated to that CPU."
Introduce the CHANNELMSG_MODIFYCHANNEL_RESPONSE(24) message type,
which embodies the type of the CHANNELMSG_MODIFYCHANNEL ACK.
Signed-off-by: Andrea Parri (Microsoft)
---
drivers/hv/channel.c | 108 --
drivers/hv/channel_mgmt.c | 42 +++
drivers/hv/connection.c | 3 +-
drivers/hv/hv.c | 52 ++
drivers/hv/hv_trace.h | 15 ++
drivers/hv/vmbus_drv.c| 4 +-
include/linux/hyperv.h| 13 -
7 files changed, 218 insertions(+), 19 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index fbdda9938039a..6801d89a20051 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -209,31 +209,107 @@ int vmbus_send_tl_connect_request(const guid_t
*shv_guest_servie_id,
}
EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
+static int send_modifychannel_without_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel msg;
+ int ret;
+
+ memset(, 0, sizeof(msg));
+ msg.header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg.child_relid = channel->offermsg.child_relid;
+ msg.target_vp = target_vp;
+
+ ret = vmbus_post_msg(, sizeof(msg), true);
+ trace_vmbus_send_modifychannel(, ret);
+
+ return ret;
+}
+
+static int send_modifychannel_with_ack(struct vmbus_channel *channel, u32
target_vp)
+{
+ struct vmbus_channel_modifychannel *msg;
+ struct vmbus_channel_msginfo *info;
+ unsigned long flags;
+ int ret;
+
+ info = kzalloc(sizeof(struct vmbus_channel_msginfo) +
+ sizeof(struct vmbus_channel_modifychannel),
+ GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ init_completion(>waitevent);
+ info->waiting_channel = channel;
+
+ msg = (struct vmbus_channel_modifychannel *)info->msg;
+ msg->header.msgtype = CHANNELMSG_MODIFYCHANNEL;
+ msg->child_relid = channel->offermsg.child_relid;
+ msg->target_vp = target_vp;
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_add_tail(>msglistentry, _connection.chn_msg_list);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ if (channel->rescind) {
+ ret = -ENODEV;
+ goto free_info;
+ }
+
+ ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_modifychannel),
true);
+ trace_vmbus_send_modifychannel(msg, ret);
+ if (ret != 0)
+ goto clean_msglist;
+
+ /*
+* Release channel_mutex; otherwise, vmbus_onoffer_rescind() could
block on
+* the mutex and be unable to signal the completion.
+*/
+ mutex_unlock(_connection.channel_mutex);
+ wait_for_completion(>waitevent);
+ mutex_lock(_connection.channel_mutex);
+
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+
+ if (channel->rescind) {
+ ret = -ENODEV;
+ goto free_info;
+ }
+
+ if (info->response.modify_response.status) {
+ kfree(info);
+ return -EAGAIN;
+ }
+
+ kfree(info);
+ return 0;
+
+clean_msglist:
+ spin_lock_irqsave(_connection.channelmsg_lock, flags);
+ list_del(>msglistentry);
+ spin_unlock_irqrestore(_connection.channelmsg_lock, flags);
+free_info:
+ kfree(info);
+ return ret;
+}
+
/*
* Set/change the vCPU (@target_vp) the channel (@child_relid) will interrupt.
*
* CHANNELMSG_MODIFYCHANNEL messages are aynchronous. Also, Hyper-V does not
- * ACK such messages. IOW we can't know when the host will stop interrupting
- * the "old" vCPU and start interrupting the "new" vCPU for the given channel.
+ * ACK such messages before VERSION_WIN10_V5_3. Without ACK, we can not know
+ * when the host will stop interrupting the "old" vCPU and start interrupting
+ * the "new" vCPU for the given channel.
*
* The CHANNELMSG_MODIFYCHANNEL message type is supported since VMBus version
* VERSION_WIN10_V4_1.
*/
-int vmbus_send_modifychannel(
Re: [PATCH 4/6] Drivers: hv: vmbus: Avoid use-after-free in vmbus_onoffer_rescind()
On Tue, Nov 24, 2020 at 04:26:33PM +, 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 v2] Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer
On Mon, Nov 09, 2020 at 11:07:27AM +0100, Andrea Parri (Microsoft) wrote:
> From: Andres Beltran
>
> Pointers to ring-buffer packets sent by Hyper-V are used within the
> guest VM. Hyper-V can send packets with erroneous values or modify
> packet fields after they are processed by the guest. To defend
> against these scenarios, return a copy of the incoming VMBus packet
> after validating its length and offset fields in hv_pkt_iter_first().
> In this way, the packet can no longer be modified by the host.
>
> Signed-off-by: Andres Beltran
> Co-developed-by: Andrea Parri (Microsoft)
> Signed-off-by: Andrea Parri (Microsoft)
> Cc: "David S. Miller"
> Cc: Jakub Kicinski
> Cc: "James E.J. Bottomley"
> Cc: "Martin K. Petersen"
> Cc: net...@vger.kernel.org
> Cc: linux-s...@vger.kernel.org
Ping, other suggestions about this patch?
Thanks,
Andrea
> ---
> Applies on 5.10-rc3 with the series:
> https://lkml.kernel.org/r/20201109100402.8946-1-parri.and...@gmail.com
>
> drivers/hv/channel.c | 9 ++--
> drivers/hv/hv_fcopy.c | 1 +
> drivers/hv/hv_kvp.c | 1 +
> drivers/hv/hyperv_vmbus.h | 2 +-
> drivers/hv/ring_buffer.c | 85 +++
> drivers/net/hyperv/hyperv_net.h | 3 ++
> drivers/net/hyperv/netvsc.c | 2 +
> drivers/net/hyperv/rndis_filter.c | 2 +
> drivers/scsi/storvsc_drv.c| 10
> include/linux/hyperv.h| 48 ++---
> net/vmw_vsock/hyperv_transport.c | 4 +-
> 11 files changed, 142 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 6fb0c76bfbf81..0d63862d65518 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -597,12 +597,15 @@ static int __vmbus_open(struct vmbus_channel
> *newchannel,
> newchannel->onchannel_callback = onchannelcallback;
> newchannel->channel_callback_context = context;
>
> - err = hv_ringbuffer_init(>outbound, page, send_pages);
> + if (!newchannel->max_pkt_size)
> + newchannel->max_pkt_size = VMBUS_DEFAULT_MAX_PKT_SIZE;
> +
> + err = hv_ringbuffer_init(>outbound, page, send_pages, 0);
> if (err)
> goto error_clean_ring;
>
> - err = hv_ringbuffer_init(>inbound,
> - [send_pages], recv_pages);
> + err = hv_ringbuffer_init(>inbound, [send_pages],
> + recv_pages, newchannel->max_pkt_size);
> if (err)
> goto error_clean_ring;
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 59ce85e00a028..660036da74495 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -349,6 +349,7 @@ int hv_fcopy_init(struct hv_util_service *srv)
> {
> recv_buffer = srv->recv_buffer;
> fcopy_transaction.recv_channel = srv->channel;
> + fcopy_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 2;
>
> /*
>* When this driver loads, the user level daemon that
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index b49962d312cef..c698592b83e42 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -757,6 +757,7 @@ hv_kvp_init(struct hv_util_service *srv)
> {
> recv_buffer = srv->recv_buffer;
> kvp_transaction.recv_channel = srv->channel;
> + kvp_transaction.recv_channel->max_pkt_size = HV_HYP_PAGE_SIZE * 4;
>
> /*
>* When this driver loads, the user level daemon that
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 02f3e89888366..e2064bf2b557d 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -174,7 +174,7 @@ extern int hv_synic_cleanup(unsigned int cpu);
> void hv_ringbuffer_pre_init(struct vmbus_channel *channel);
>
> int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
> -struct page *pages, u32 pagecnt);
> +struct page *pages, u32 pagecnt, u32 max_pkt_size);
>
> void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 35833d4d1a1dc..bd70b7f1ffe60 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -190,7 +190,7 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)
>
> /* Initialize the ring buffer. */
> int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
> -struct page *pages, u32 page_cnt)
> +struct page *pages, u32 page_cnt, u32 max_pkt_size)
>
Re: [PATCH v4] hv_utils: Add validation for untrusted Hyper-V values
On Mon, Nov 09, 2020 at 11:07:04AM +0100, Andrea Parri (Microsoft) wrote:
> From: Andres Beltran
>
> For additional robustness in the face of Hyper-V errors or malicious
> behavior, validate all values that originate from packets that Hyper-V
> has sent to the guest in the host-to-guest ring buffer. Ensure that
> invalid values cannot cause indexing off the end of the icversion_data
> array in vmbus_prep_negotiate_resp().
>
> Signed-off-by: Andres Beltran
> Co-developed-by: Andrea Parri (Microsoft)
> Signed-off-by: Andrea Parri (Microsoft)
Ping, other suggestions about this patch?
Thanks,
Andrea
> ---
> Changes in v3:
> - Add size check for icframe_vercnt and icmsg_vercnt
>
> Changes in v2:
> - Use ratelimited form of kernel logging to print error messages
>
> drivers/hv/channel_mgmt.c | 24 -
> drivers/hv/hv_fcopy.c | 36 +--
> drivers/hv/hv_kvp.c | 122 -
> drivers/hv/hv_snapshot.c | 89 ---
> drivers/hv/hv_util.c | 222 +++---
> include/linux/hyperv.h| 9 +-
> 6 files changed, 314 insertions(+), 188 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 1d44bb635bb84..5bc5eef5da159 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -190,6 +190,7 @@ static u16 hv_get_dev_type(const struct vmbus_channel
> *channel)
> * vmbus_prep_negotiate_resp() - Create default response for Negotiate
> message
> * @icmsghdrp: Pointer to msg header structure
> * @buf: Raw buffer channel data
> + * @buflen: Length of the raw buffer channel data.
> * @fw_version: The framework versions we can support.
> * @fw_vercnt: The size of @fw_version.
> * @srv_version: The service versions we can support.
> @@ -202,8 +203,8 @@ static u16 hv_get_dev_type(const struct vmbus_channel
> *channel)
> * Set up and fill in default negotiate response message.
> * Mainly used by Hyper-V drivers.
> */
> -bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp,
> - u8 *buf, const int *fw_version, int fw_vercnt,
> +bool vmbus_prep_negotiate_resp(struct icmsg_hdr *icmsghdrp, u8 *buf,
> + u32 buflen, const int *fw_version, int
> fw_vercnt,
> const int *srv_version, int srv_vercnt,
> int *nego_fw_version, int *nego_srv_version)
> {
> @@ -215,10 +216,14 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
> bool found_match = false;
> struct icmsg_negotiate *negop;
>
> + /* Check that there's enough space for icframe_vercnt, icmsg_vercnt */
> + if (buflen < ICMSG_HDR + offsetof(struct icmsg_negotiate, reserved)) {
> + pr_err_ratelimited("Invalid icmsg negotiate\n");
> + return false;
> + }
> +
> icmsghdrp->icmsgsize = 0x10;
> - negop = (struct icmsg_negotiate *)[
> - sizeof(struct vmbuspipe_hdr) +
> - sizeof(struct icmsg_hdr)];
> + negop = (struct icmsg_negotiate *)[ICMSG_HDR];
>
> icframe_major = negop->icframe_vercnt;
> icframe_minor = 0;
> @@ -226,6 +231,15 @@ bool vmbus_prep_negotiate_resp(struct icmsg_hdr
> *icmsghdrp,
> icmsg_major = negop->icmsg_vercnt;
> icmsg_minor = 0;
>
> + /* Validate negop packet */
> + if (icframe_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> + icmsg_major > IC_VERSION_NEGOTIATION_MAX_VER_COUNT ||
> + ICMSG_NEGOTIATE_PKT_SIZE(icframe_major, icmsg_major) > buflen) {
> + pr_err_ratelimited("Invalid icmsg negotiate - icframe_major:
> %u, icmsg_major: %u\n",
> +icframe_major, icmsg_major);
> + goto fw_error;
> + }
> +
> /*
>* Select the framework version number we will
>* support.
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 5040d7e0cd9e9..59ce85e00a028 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -235,15 +235,27 @@ void hv_fcopy_onchannelcallback(void *context)
> if (fcopy_transaction.state > HVUTIL_READY)
> return;
>
> - vmbus_recvpacket(channel, recv_buffer, HV_HYP_PAGE_SIZE * 2, ,
> - );
> - if (recvlen <= 0)
> + if (vmbus_recvpacket(channel, recv_buffer, HV_HYP_PAGE_SIZE * 2,
> , )) {
> + pr_err_ratelimited("Fcopy request received. Could not read into
> recv buf\n");
> return;
> + }
> +
> + if (!recvlen)
> +
