[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 <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: x86@kernel.org
Cc: linux-arch@vger.kernel.org
Cc: netdev@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

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) <parri.andrea@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@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 <linux/prefetch.h>
 
 #include <asm/sync_bitops.h>
+#include <asm/mshyperv.h>
 
 #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 = &net_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

Please also add a log there stating we don't support sriov in this case. Otherwise,
customers will ask why vf not showing up.

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.

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

When the driver doesn't advertise SRIOV, this message is not expected.
Instead of ignore silently, we should log an error.

Thanks,
- Haiyang

On Tue, 19 Jan 2021 18:58:41 +0100 Andrea Parri (Microsoft) wrote:

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) <parri.andrea@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: netdev@vger.kernel.org

Nothing exciting here from networking perspective, so:

Acked-by: Jakub Kicinski <kuba@kernel.org>

quoted

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

quoted

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.

quoted

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

quoted

+}
+
 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 = &net_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)

quoted

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

-----Original Message-----
From: Andrea Parri <parri.andrea@gmail.com>
Sent: Wednesday, January 20, 2021 11:05 PM
To: Haiyang Zhang <haiyangz@microsoft.com>
Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
Stephen Hemminger [off-list ref]; Wei Liu
[off-list ref]; Michael Kelley [off-list ref]; Tianyu Lan
[off-list ref]; Saruhan Karademir
[off-list ref]; Juan Vazquez [off-list ref]; linux-
hyperv@vger.kernel.org; David S. Miller [off-list ref]; Jakub
Kicinski [off-list ref]; netdev@vger.kernel.org
Subject: Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated
guests

quoted

quoted

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

quoted

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,

quoted

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"

quoted

quoted

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

quoted

quoted

+}
+
 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 = &net_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.

quoted

I'd suggest move this check after version negotiation is done. So we can log

what's

quoted

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

Please point out this invalid version is caused by the host side, like this:
	"Invalid NVSP version 0x50000  (expected >= 0x60001) from the host with isolation support"
Also please use "0x%x" for hexadecimal numbers.

quoted

quoted

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

Thanks,
- Haiyang

quoted

quoted

quoted

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

quoted

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,

quoted

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.

quoted

quoted

quoted

@@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device

*device,
 	init_packet = &net_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.

quoted

I'd suggest move this check after version negotiation is done. So we can log

what's

quoted

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 60000  (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 0x50000  (expected >= 0x60001) from the host with isolation support"
Also please use "0x%x" for hexadecimal numbers.

Sure.

quoted

quoted

quoted

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