| Message ID | 20210119175841.22248-5-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Drivers: hv: vmbus: Restrict devices and configurations on 'isolated' guests | expand |
> -----Original Message----- > From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > Sent: Tuesday, January 19, 2021 12:59 PM > To: linux-kernel@vger.kernel.org > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; Stephen Hemminger > <sthemmin@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Michael Kelley > <mikelley@microsoft.com>; Tianyu Lan <Tianyu.Lan@microsoft.com>; > Saruhan Karademir <skarade@microsoft.com>; Juan Vazquez > <juvazq@microsoft.com>; linux-hyperv@vger.kernel.org; Andrea Parri > (Microsoft) <parri.andrea@gmail.com>; David S. Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > netdev@vger.kernel.org > 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@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; 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
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>
> > @@ -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 = &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) > > @@ -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 <sthemmin@microsoft.com>; Wei Liu > <wei.liu@kernel.org>; Michael Kelley <mikelley@microsoft.com>; Tianyu Lan > <Tianyu.Lan@microsoft.com>; Saruhan Karademir > <skarade@microsoft.com>; Juan Vazquez <juvazq@microsoft.com>; linux- > hyperv@vger.kernel.org; David S. Miller <davem@davemloft.net>; Jakub > Kicinski <kuba@kernel.org>; netdev@vger.kernel.org > Subject: 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" > > > > > @@ -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 = &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) 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. > > > > > @@ -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
> > > > @@ -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 = &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) > > 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. > > > > @@ -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
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; } }
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(-)