RE: [PATCH v4 net-next 01/15] net: enetc: add trusted VF support

[Wei Fang]

> > @@ -574,6 +578,7 @@ static const struct net_device_ops enetc4_ndev_ops
> = {
> >  	.ndo_eth_ioctl		= enetc_ioctl,
> >  	.ndo_hwtstamp_get	= enetc_hwtstamp_get,
> >  	.ndo_hwtstamp_set	= enetc_hwtstamp_set,
> > +	.ndo_set_vf_trust	= enetc_pf_set_vf_trust,
> >  };
> 
> [Severity: Low]
> At this commit enetc4_pf_driver has no .sriov_configure member and enetc4
> never calls enetc_msg_psi_init(), so VFs cannot be instantiated on ENETC v4
> and the mailbox worker is never armed.
> 
> The only reader of ENETC_VF_FLAG_TRUSTED is
> enetc_msg_set_vf_primary_mac_addr(), reached via enetc_msg_task() which is
> only set up by enetc_sriov_configure().  Does that mean
> 
>   ip link set <enetc4 pf> vf N trust on
> 
> returns 0 here with no observable effect, and that the new
> enetc_init_sriov_resources() call allocates rxmsg/vf_state plus total_vfs
> mutexes for slots that cannot exist yet?
> 
> This is resolved later in the same series: the patch adding VF support for
> i.MX94 and i.MX95 adds .sriov_configure = enetc_sriov_configure to
> enetc4_pf_driver, and enetc_msg.c then gates several ENETC v4 mailbox
> operations on the trust flag.  Noting it only because the knob is advertised
> one patch before it can do anything.

No need fix, this patch is a preparatory step for ENETC v4 VF to support
MAC filters.

> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > index edc1277bb586..78114ab3e482 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > @@ -61,31 +63,49 @@ static u16
> enetc_msg_set_vf_primary_mac_addr(struct enetc_pf *pf, int vf_id,
> >  	struct enetc_vf_state *vf_state = &pf->vf_state[vf_id];
> >  	struct enetc_msg_mac_exact_filter *msg = vf_msg;
> >  	struct device *dev = &pf->si->pdev->dev;
> > +	u16 pf_msg = ENETC_PF_MSG_SUCCESS;
> >  	char *addr = msg->mac[0].addr;
> >
> > +	mutex_lock(&vf_state->lock);
> > +
> > +	/* Untrusted VFs cannot set their MAC addresses by the mailbox
> > +	 * messages.
> > +	 */
> > +	if (!(vf_state->flags & ENETC_VF_FLAG_TRUSTED)) {
> > +		pf_msg = ENETC_PF_MSG_PERM_DENY;
> > +		goto vf_state_unlock;
> > +	}
> > +
> 
> [Severity: Medium]
> Is this a user-visible behaviour change for existing SR-IOV deployments?

The VF will return error code when it has no privilege.

> 
> ENETC_VF_FLAG_TRUSTED is zero for every slot, since vf_state is
> devm_kcalloc()'d in enetc_init_sriov_resources() and only
> "ip link set <pf> vf N trust on" sets the bit.  On the VF side
> enetc_msg_vsi_send() maps the new response to an errno:
> 
>   case ENETC_MSG_CLASS_ID_PERMISSION_DENY:
> 	return -EACCES;
> 
> So after this patch enetc_vf_set_mac_addr() -> enetc_msg_vsi_send() fails
> with -EACCES, and an in-guest "ip link set <vf> address ..." or enslaving
> the VF into a bond/macvlan stops working after a kernel upgrade with no
> configuration change.
> 
> The commit message says:
> 
>     This does not regress VF bring-up. The PF programs a valid
>     primary MAC into every VF slot at probe, and the VF loads it from its
>     own SIPMAR0/1 registers.
> 
> Bring-up is indeed unaffected, but could the message also state that guests
> which previously changed their own VF MAC now need the host to mark the slot
> trusted first?
> 
> Related question on tags: the message describes this as preventing a
> malicious VM from eavesdropping on other SIs and from spoofing MAC
> addresses, and the VF-set-MAC path has existed since commit beb74ac878c8
> ("enetc: Add vf to pf messaging support").  Should this carry a Fixes: tag
> or stable guidance for backporters?

This is a new feature for both ENETC v1 and v4, I do not think we need a
Fixes tag for new feature.

> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > index 55c07c528f22..a7bf4bfc25b7 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> > @@ -488,6 +488,7 @@ static const struct net_device_ops enetc_ndev_ops =
> {
> >  	.ndo_set_rx_mode	= enetc_pf_set_rx_mode,
> >  	.ndo_vlan_rx_add_vid	= enetc_vlan_rx_add_vid,
> >  	.ndo_vlan_rx_kill_vid	= enetc_vlan_rx_del_vid,
> > +	.ndo_set_vf_trust	= enetc_pf_set_vf_trust,
> >  	.ndo_set_vf_mac		= enetc_pf_set_vf_mac,
> >  	.ndo_set_vf_vlan	= enetc_pf_set_vf_vlan,
> >  	.ndo_set_vf_spoofchk	= enetc_pf_set_vf_spoofchk,
> 
> [Severity: Low]
> At this commit neither enetc_ndev_ops nor enetc4_ndev_ops implements
> .ndo_get_vf_config, and rtnl_fill_vf() returns early when
> ndo_get_vf_config is NULL, so IFLA_VF_TRUST is never emitted and
> "ip link show <pf>" prints no VF block.
> 
> Since the remediation the commit message recommends is
> "ip link set <dev> vf <N> trust off", would it be worth making the trust
> state readable in the same patch that makes it settable?
> 
> This is closed later in the series by the patch adding ndo_get_vf_config()
> support, which reports ivi->trusted from the same flag.

No need fix, the subsequent patch in this series adds ndo_get_vf_config()
support.

> > +	vf_state = &pf->vf_state[vf];
> > +	mutex_lock(&vf_state->lock);
> > +
> > +	if (setting)
> > +		vf_state->flags |= ENETC_VF_FLAG_TRUSTED;
> > +	else
> > +		vf_state->flags &= ~ENETC_VF_FLAG_TRUSTED;
> 
> [Severity: High]
> Does clearing trust here leave the hardware MAC filter that the VF
> programmed while it was trusted?
> 
> While trusted, the guest can put any valid unicast address into the SI
> primary MAC registers, including the PF's or another VF's:

A trusted host means it should not change its MAC address to a PF or other
VF address. If it needs to sniff traffic from other VFs or PFs, it can enable
promiscuous mode. So this is not an expected behavior of a trusted VF.

> 
> drivers/net/ethernet/freescale/enetc/enetc_msg.c:enetc_msg_set_vf_primary
> _mac_addr() {
> 	...
> 	enetc_set_si_hw_addr(pf, vf_id + 1, addr);
> 	...
> }
> 
> The trust-off branch above only clears the software bit, so ENETC_PSIPMAR0/1
> (ENETC4_PSIPMAR0/1 on v4) for that SI keep the guest-chosen address.
> 
> The other paths that could restore it do not.  enetc_setup_mac_addresses()
> is the only place a PF-chosen MAC is written into every VF slot, and it runs
> at probe:
> 
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:enetc_setup_mac_
> addresses() {
> 	...
> 	for (i = 0; i < pf->total_vfs; i++) {
> 		err = enetc_setup_mac_address(NULL, pf, i + 1);
> 	...
> }
> 
> enetc4_pf_vf_flr_handler() re-applies only promiscuous state, so the address
> also survives a VF FLR (guest reboot or driver reload), and the SR-IOV
> disable path does not touch it either.
> 
> So after the documented "ip link set <dev> vf <N> trust off", frames for the
> spoofed address still hit that SI's exact-match filter, and a slot later
> reassigned to a different guest inherits the address, because the VF driver
> reads its MAC out of SIPMAR0/1.
> 
> At the end of the series the trust-off branch grows scrubbing of
> promiscuous mode and of the UC/MC hash filters, but still does not restore
> the SI primary MAC.  Should this branch reprogram the PF-assigned primary
> MAC for the slot as well?

This follows the same intentional model as the trust gate itself.

Once a VF is set to untrusted, it can no longer change its MAC at all - any
subsequent ndo_set_mac_address from the VF is immediately rejected with
PERM_DENY. So the MAC left in the SI primary filter is simply the address the
host authorized while the VF was trusted; it is not something the now-untrusted
VF can keep modifying.

The host also retains a definitive way to reclaim or reset that address:
ip link set ... vf N mac <addr> (ndo_set_vf_mac) reprograms PSIPMAR0/1 and
sets ENETC_VF_FLAG_PF_SET_MAC, after which even a trusted VF can no longer
override it. So there is no need to save the probe-time address or to force a VF
reset - the host can deterministically take over the primary MAC through the
existing PF interface.