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

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

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

- [High] Trust revocation ("ip link set <dev> vf <N> trust off") clears
  only the software bit in pf->vf_state[vf].flags; it never reprograms…
- [Medium] Default-deny of the VF primary-MAC mailbox message is a user-
  visible behaviour change that the commit message understates, and the…
- [Low] Reported for completeness and marked as resolved within the
  series: at this commit both enetc_ndev_ops (enetc_pf.c:482-503) and…
- [Low] Reported for completeness and marked as resolved within the
  series: at this commit enetc4_ndev_ops advertises .ndo_set_vf_trust…
- [Low] Reported for completeness and assessed as intended, documented
  behaviour: ENETC_VF_FLAG_TRUSTED lives in pf->vf_state[] which is…

--

net: enetc: add trusted VF support

This adds an ENETC_VF_FLAG_TRUSTED per-VF flag and wires up
ndo_set_vf_trust through the new shared enetc_pf_set_vf_trust() helper for
both the enetc and enetc4 PF drivers.  The first mailbox message gated on
the flag is the VF primary MAC address change, which now returns
ENETC_MSG_CLASS_ID_PERMISSION_DENY for untrusted VFs.

A few questions below.

[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.

[Severity: Medium]
Is this a user-visible behaviour change for existing SR-IOV deployments?

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?

 	if (!is_valid_ether_addr(addr)) {
 		dev_err_ratelimited(dev, "VF%d attempted to set invalid MAC\n",
 				    vf_id);
-		return (FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
-				   ENETC_MSG_CLASS_ID_MAC_FILTER) |
-			FIELD_PREP(ENETC_PF_MSG_CLASS_CODE,
-				   ENETC_MF_CLASS_CODE_INVALID_MAC));
+		pf_msg = FIELD_PREP(ENETC_PF_MSG_CLASS_ID,
+				    ENETC_MSG_CLASS_ID_MAC_FILTER) |
+			 FIELD_PREP(ENETC_PF_MSG_CLASS_CODE,
+				    ENETC_MF_CLASS_CODE_INVALID_MAC);
+		goto vf_state_unlock;
 	}
 
-	mutex_lock(&vf_state->lock);
+	/* PF has higher privileges. If PF has already modified the MAC
+	 * address for VF through .ndo_set_vf_mac() interface, VF is not
+	 * allowed to set its MAC address via mailbox messages, even if
+	 * it is trusted.
+	 */
 	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);
+
+vf_state_unlock:
 	mutex_unlock(&vf_state->lock);
 
-	return ENETC_PF_MSG_SUCCESS;
+	return pf_msg;
 }

[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.

[Severity: Low]
Trust lives in pf->vf_state[], which is allocated once at PF probe, and the
bound here is pf->total_vfs rather than pf->num_vfs.  It is not cleared by
enetc_sriov_configure(pdev, 0), by pci_disable_sriov(), or by VF FLR, so a
slot trusted for one guest stays trusted when the VF is destroyed and
re-created for another guest.

The commit message states this is deliberate:

    it is intentionally preserved across an SR-IOV disable/enable
    cycle. The bounds check uses pf->total_vfs, the hardware maximum and
    the size of pf->vf_state[], so pre-configuring an uninstantiated slot
    is in-bounds. If a slot may be reassigned to another guest, the admin
    clears trust with "ip link set <dev> vf <N> trust off".

Later in the series enetc_msg_clear_vf_config() encodes the same rationale
in a comment, so this looks intended and is mentioned only for the record.

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

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?

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