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

quoted

@@ -574,6 +578,7 @@ static const struct net_device_ops enetc4_ndev_ops

= {

quoted

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

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c

b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

quoted

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,

quoted

 	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.

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c

b/drivers/net/ethernet/freescale/enetc/enetc_pf.c

quoted

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 =

{

quoted

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

quoted

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