linux-kernel - Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210121040526.GA264889@anparri>
Date:   Thu, 21 Jan 2021 05:05:26 +0100
From:   Andrea Parri <parri.andrea@...il.com>
To:     Haiyang Zhang <haiyangz@...rosoft.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        Michael Kelley <mikelley@...rosoft.com>,
        Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        Saruhan Karademir <skarade@...rosoft.com>,
        Juan Vazquez <juvazq@...rosoft.com>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.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.


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