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

["Wei Fang (OSS)" <wei.fang@oss.nxp.com>]

> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> > index edc1277bb5860..78114ab3e4820 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: High]
> Does this code break standard Linux SR-IOV semantics?
> 
> By unconditionally returning ENETC_PF_MSG_PERM_DENY for untrusted VFs,
> VMs and
> container frameworks that rely on the standard behavior may fail to initialize
> networking. Specifically, untrusted VMs that generate and set their own MAC
> (when the host has not enforced one) will receive a permission denied response
> and fail. Standard drivers typically allow untrusted VFs to set their MAC if
> no administrative MAC was assigned by the host.

This is a false positive. The PF allocates the MAC address for each VF by default.
The VF can get the MAC address from its SIPMAR0/1 registers. So untrusted VF
still can use the network.

The current patch just prevents the untrusted VF from setting its own MAC
address through the mailbox channel.

> 
> > +
> >  	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);
> 
> [Severity: High]
> Is this restriction defeating the established kernel purpose of the VF_TRUST
> flag?
> 
> If a VF is trusted, standard SR-IOV semantics typically allow it to override
> administrative restrictions like a host-assigned MAC address. This block
> prevents even trusted VFs from overriding a PF-assigned MAC address, causing
> a trusted VF to fail with a command not permitted error if it tries to change
> a host-assigned MAC.

This is also a false positive. This part is inherited from LS1028A, and is not
introduced by this patch. This patch simply adds some comments explaining
the intention behind doing so. For trusted VFs, the PF still has the highest
privileges. If a user assigns an address to a VF via the ndo_set_vf_mac()
interface, even a trusted VF cannot overwrite the MAC address set by the PF.