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

[Wei Fang]

> This adds a per-VF ENETC_VF_FLAG_TRUSTED flag stored in pf->vf_state[] and
> wires up ndo_set_vf_trust through the new shared helper
> enetc_pf_set_vf_trust() for both the enetc and enetc4 PF drivers.  The VF
> primary MAC address mailbox message is the first message gated on that
> flag, and untrusted VFs now get ENETC_MSG_CLASS_ID_PERMISSION_DENY
> instead
> of having the hardware programmed.
> 
> A couple of questions about the trust flag lifetime and about the
> changelog below.
> 
> The changelog says:
> 
>     An untrusted VF that attempts to set its own MAC address will receive a
>     ENETC_MSG_CLASS_ID_PERMISSION_DENY response and the hardware
> will not
>     be programmed. To prevent a malicious VM from setting the VF address
> to
>     the MAC address of other VFs or PF, thereby eavesdropping on the traffic
>     of other SIs.
> 
> [Severity: Low]
> Before this patch any VF address that passed is_valid_ether_addr() and the
> ENETC_VF_FLAG_PF_SET_MAC check was programmed into the SI primary
> MAC
> registers, so the description matches the pre-patch behaviour.  Was a Fixes
> tag considered here, given the wording describes an exploitable condition?

No need a fixes tag, the intention of ndo_set_vf_trust() is a new feature not a
bug fix. ENETC v1 is only used on LS1028A, but due to the hardware issue (MDIO
issue), the VFs must run on the same OS with PF, so for ENETC v1, VF cannot be
assigned to VM.

> >  static void enetc_msg_disable_mr_int(struct enetc_pf *pf)
> >  {
> > @@ -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;
> > +	}
> > +
> >  	if (!is_valid_ether_addr(addr)) {
> >  		dev_err_ratelimited(dev, "VF%d attempted to set invalid MAC\n",
> >  				    vf_id);
> 
> [Severity: High]
> This gate looks like it inverts the usual meaning of the trust
> attribute, and it changes behaviour for existing VF users.
> 
> In ixgbe/i40e/ice the rule is: a VF may program its own MAC as long as
> the PF has not administratively assigned one, and "trusted" is exactly
> the knob that lets a VF go further and override the PF-assigned address.
> Here both halves are reversed. The new check in
> enetc_msg_set_vf_primary_mac_addr() returns ENETC_PF_MSG_PERM_DENY
> for
> any VF without ENETC_VF_FLAG_TRUSTED, even when no PF-assigned address
> exists (ENETC_VF_FLAG_PF_SET_MAC clear), and the following
> ENETC_VF_FLAG_PF_SET_MAC test still refuses the request for a trusted
> VF - the new comment states this explicitly ("even if it is trusted").
> So trust neither grants the override it is supposed to grant, nor is it
> irrelevant to the plain "VF picks its own MAC" case.
> 
> Since vf_state[] is devm_kcalloc()'ed in enetc_init_sriov_resources()
> and flags starts at 0, every VF is untrusted at creation. That means
> after this patch a guest that generates its own address and calls
> ndo_set_mac_address on the VF gets the deny class back, which
> enetc_msg_vsi_send() turns into -EACCES, and interface bring-up fails
> unless the host has first run "ip link set ... vf N trust on". That is
> a visible change for setups that work today, and there is no Fixes tag
> or mention of the compatibility impact in the changelog.
> 
> Was the ixgbe-style rule considered instead, i.e. allow the mailbox MAC
> change whenever ENETC_VF_FLAG_PF_SET_MAC is clear, and consult
> ENETC_VF_FLAG_TRUSTED only to decide whether a VF may override a
> PF-assigned address? If the stricter default-deny is deliberate for
> this hardware, could the changelog spell out that untrusted VFs can no
> longer set any MAC at all and that trust is now required for what used
> to work by default?

This behavior is intentional, and the compatibility impact is minimal. A few
points to clarify:

1. No bring-up regression. The PF driver assigns a MAC address to every VF
at probe time, so each VF reads a valid address from its own SIPMAR0/1
registers via enetc_load_primary_mac_addr(). Interface bring-up (.ndo_open)
does not send any set-MAC mailbox message, so it is unaffected. Only an
explicit ndo_set_mac_address (a VF trying to change its MAC via mailbox)
is now gated on trust.

2. The default-deny is deliberate, not an inverted trust rule. It prevents a
malicious VM from setting its MAC to that of another VF/PF to eavesdrop
on other SIs' traffic, or from spoofing a MAC to bypass security policies. So
the two gates are complementary: untrusted VFs cannot self-assign a MAC
at all, and even a trusted VF cannot override a PF-assigned MAC. This is a
stricter model than ixgbe/i40e/ice by design, which suits the ENETC SI
security model.

3. Impact on existing SoCs is negligible. Due to the ENETC v1 MDIO issue,
ENETC v1 VFs cannot be assigned to a VM, and the v1 mailbox path is
effectively unusable downstream. This change is primarily aimed at the
upcoming ENETC v4 VF support, so it does not change behavior for any
real existing deployment.

> > 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,
> 
> [Severity: Low]
> At this commit neither enetc_ndev_ops nor enetc4_ndev_ops provides
> .ndo_get_vf_config, so rtnl_fill_vfinfo() emits no VF info and the trust
> bit set here cannot be read back with "ip link show".
> 
> This does get resolved inside the same series: the later commit "net:
> enetc: add ndo_get_vf_config() support" adds enetc_pf_get_vf_config() with
> 
> 	ivi->trusted = !!(vf_state->flags & ENETC_VF_FLAG_TRUSTED);
> 
> and registers it in both ops tables.  Would it be worth ordering the getter
> before or together with the setter so that no intermediate commit has a
> write-only trust attribute?

Since these two patches are in the same series, there is no need to put them
together.

> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > index d32a195a04c9..519fc90d2647 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > @@ -586,5 +586,28 @@ int enetc_init_sriov_resources(struct enetc_pf *pf)
> >  }
> >  EXPORT_SYMBOL_GPL(enetc_init_sriov_resources);
> >
> > +int enetc_pf_set_vf_trust(struct net_device *ndev, int vf, bool setting)
> > +{
> > +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> > +	struct enetc_vf_state *vf_state;
> > +
> > +	if (vf >= pf->total_vfs)
> > +		return -EINVAL;
> > +
> > +	vf_state = &pf->vf_state[vf];
> > +	mutex_lock(&vf_state->lock);
> > +
> > +	if (setting)
> > +		vf_state->flags |= ENETC_VF_FLAG_TRUSTED;
> 
> [Severity: Medium]
> Can ENETC_VF_FLAG_TRUSTED survive an SR-IOV teardown and be inherited
> by a
> different VF instance later?
> 
> pf->vf_state[] is devm_kcalloc()'ed once per PF in
> enetc_init_sriov_resources(), and the disable path in
> enetc_sriov_configure() does not touch it:
> 
> 	if (!num_vfs) {
> 		pci_disable_sriov(pdev);
> 		enetc_msg_psi_free(pf);
> 		pf->num_vfs = 0;
> 	} else {
> 
> So for:
> 
> 	echo 2 > sriov_numvfs
> 	ip link set eth0 vf 0 trust on
> 	echo 0 > sriov_numvfs
> 	echo 2 > sriov_numvfs
> 
> vf_state[0].flags still has ENETC_VF_FLAG_TRUSTED set, and the newly
> created VF0 (possibly assigned to a different guest) passes the new gate in
> enetc_msg_set_vf_primary_mac_addr() straight away and gets
> enetc_set_si_hw_addr(pf, vf_id + 1, addr) executed for whatever address it
> asks for.
> 
> Related: the bounds check here uses pf->total_vfs, the hardware maximum,
> rather than pf->num_vfs, so trust can also be recorded for VF slots that do
> not exist yet and takes effect as soon as they are created.  Was that
> intentional?
> 
> Later in the series enetc_msg_clear_vf_config() clears only the UC/MC
> promiscuous bits and carries a comment saying TRUSTED and PF_SET_MAC are
> intentionally preserved, and it returns early for ENETC v1, so the
> carry-over is still present at the end of the series.  Would clearing the
> trust bit for all VFs on SR-IOV disable, as ixgbe and i40e effectively do
> by reallocating their per-VF state, be preferable?

This is intentional by design. The ENETC_VF_FLAG_TRUSTED flag is decoupled
from whether SR-IOV is enabled - it can be set even before SR-IOV is enabled.
This is deliberate: when SR-IOV is later enabled, a trusted VF can apply certain
configurations right at init time, instead of having to wait for the user to enable
the trust flag on the PF side first.

The persistence across teardown is therefore expected, not a leak. Trust is a
PF/host-side administrative policy tied to the VF index, and the host always
remains in control. If an admin is concerned about a VF slot being reused by
a different guest, the correct action is to explicitly set that VF back to untrusted
via "ip link set ... vf N trust off" after disabling SR-IOV.

The total_vfs bounds check is consistent with this model - it intentionally
allows trust to be pre-configured for VF slots before they are instantiated,
and the array is allocated for total_vfs, so there is no out-of-bounds access.

So no fix is needed here.
> 
> > +	else
> > +		vf_state->flags &= ~ENETC_VF_FLAG_TRUSTED;
> > +
> > +	mutex_unlock(&vf_state->lock);
> 
> [Severity: Medium]
> Does revoking trust leave the hardware MAC filter behind?
> 
> While trusted, the VF can install any valid unicast address, including the
> PF's or another VSI's, through enetc_msg_set_vf_primary_mac_addr() ->
> enetc_set_si_hw_addr(pf, vf_id + 1, addr).  This path only clears software
> flag bits, so after "ip link set ... vf N trust off" the SI primary MAC
> filter still holds the address the VF picked, and the now-untrusted VF keeps
> receiving frames for it.
> 
> There is also no saved copy of the address programmed at probe, so the
> original value cannot be restored.
> 
> At the end of the series the untrust branch clears UC/MC promiscuous mode
> and the hash filters (and skips even that for ENETC v1), but still never
> re-programs or clears the primary MAC.  ixgbe_ndo_set_vf_trust() and
> i40e_ndo_set_vf_trust() force a VF reset on a trust change so the VF has to
> re-negotiate its filters; would something equivalent be appropriate here?
> 

No fix is needed here. 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.

Unlike ixgbe/i40e, ENETC does not need a VF reset to regain control, because
the primary MAC is a single per-SI register the PF owns directly, rather than
something re-negotiated through a VF<->PF protocol. Clearing only the software
trust flag on trust off is therefore sufficient and intended.