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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 2

- [High] The patch breaks standard Linux SR-IOV semantics by
  unconditionally preventing untrusted VFs from setting their MAC…
- [Medium] ENETC_VF_FLAG_TRUSTED is stored in pf->vf_state[], which is
  devm_kcalloc()'ed once per PF at probe (enetc_init_sriov_resources(),…
- [Medium] Trust revocation leaves hardware state behind:
  enetc_pf_set_vf_trust() clears only software flag bits.
- [Low] (Baseline verdict: false positive - retained only because the
  concern is flagged requires_validation.) Claim: the commit describes a…
- [Low] (Baseline verdict: false positive - retained only because the
  concern is flagged requires_validation.) Claim: both PF ops tables…

--

net: enetc: add trusted VF support

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?

This is likely just a maintainer judgement call rather than a code problem:
the default-deny change is documented in the changelog itself, and the VF
side already maps the deny class to -EACCES in enetc_msg_vsi_send() with an
explicit comment, so the deny path looks like a coordinated design point.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> index 9bb1004548ab..935a6a03b14f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

[ ... ]

> @@ -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,
>  };

> 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
> @@ -7,6 +7,8 @@
>  					   ENETC_MSG_CLASS_ID_CMD_SUCCESS)
>  #define ENETC_PF_MSG_NOTSUPP	FIELD_PREP(ENETC_PF_MSG_CLASS_ID, \
>  					   ENETC_MSG_CLASS_ID_CMD_NOT_SUPPORT)
> +#define ENETC_PF_MSG_PERM_DENY	FIELD_PREP(ENETC_PF_MSG_CLASS_ID, \
> +					   ENETC_MSG_CLASS_ID_PERMISSION_DENY)
>  
>  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?

[ ... ]

>  	if (vf_state->flags & ENETC_VF_FLAG_PF_SET_MAC) {
> -		mutex_unlock(&vf_state->lock);
>  		dev_err_ratelimited(dev,
>  				    "VF%d attempted to override PF set MAC\n",
>  				    vf_id);
> -		return FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
> -				  ENETC_MSG_CLASS_ID_CMD_NOT_PERMITTED);
> +		pf_msg = FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
> +				    ENETC_MSG_CLASS_ID_CMD_NOT_PERMITTED);
> +		goto vf_state_unlock;
>  	}
>  
>  	enetc_set_si_hw_addr(pf, vf_id + 1, addr);
                ^^^^^^^^^^^^^^^^^^^^^^

Note for the two questions further down: this is where the trusted VF's
chosen address is committed to the per-SI primary MAC registers, and the
driver keeps no copy of the address that was there before.

> +
> +vf_state_unlock:
>  	mutex_unlock(&vf_state->lock);
>  
> -	return ENETC_PF_MSG_SUCCESS;
> +	return pf_msg;
>  }

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

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

> +	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?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(enetc_pf_set_vf_trust);
> +
>  MODULE_DESCRIPTION("NXP ENETC PF common functionality driver");
>  MODULE_LICENSE("Dual BSD/GPL");

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831025441.635045-1-wei.fang%40oss.nxp.com