RE: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests

[Haiyang Zhang]

> -----Original Message-----
> From: Andrea Parri (Microsoft) <parri.andrea@xxxxxxxxx>
> Sent: Tuesday, January 19, 2021 12:59 PM
> To: linux-kernel@xxxxxxxxxxxxxxx
> Cc: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>; Wei Liu <wei.liu@xxxxxxxxxx>; Michael Kelley
> <mikelley@xxxxxxxxxxxxx>; Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>;
> Saruhan Karademir <skarade@xxxxxxxxxxxxx>; Juan Vazquez
> <juvazq@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx; Andrea Parri
> (Microsoft) <parri.andrea@xxxxxxxxx>; David S. Miller
> <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> netdev@xxxxxxxxxxxxxxx
> Subject: [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) <parri.andrea@xxxxxxxxx>
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> ---
>  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;

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

> 
>  		/* 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;
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.


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

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

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

Thanks,
- Haiyang