Re: [PATCH net 2/6] net: enetc: fix race condition in VF MAC address configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing return value assignment in enetc_pf_probe() on memory allocation failure leads to false probe success and a later kernel panic.
- [High] Endianness portability bug: Hardware MAC address programming uses native-endian __raw_writel and lacks memory barriers.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) vulnerability exists when validating the VF MAC address from shared DMA memory.
- [Medium] Unbalanced lifecycle: The dynamically allocated vf_state->lock mutexes are never destroyed.
--

commit 1c1cbdf4d36cff2b27997a549d795686b791e573
Author: Wei Fang <wei.fang@nxp.com>

net: enetc: fix race condition in VF MAC address configuration

The VF message handler runs asynchronously and accesses vf_state->flags
without locking. Concurrently, the host can change the VF MAC address
via enetc_pf_set_vf_mac() under RTNL lock, creating TOCTOU races and
torn MAC address writes. Fix this by introducing a per-VF mutex to
serialize access.

Link: https://sashiko.dev/#/patchset/20260511080805.2052495-1-wei.fang%40nxp.com #1

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -252,8 +252,12 @@ static int enetc_pf_set_vf_mac(struct net_device *ndev, int vf, u8 *mac)
>  		return -EADDRNOTAVAIL;
>  
>  	vf_state = &pf->vf_state[vf];
> +
> +	mutex_lock(&vf_state->lock);
>  	vf_state->flags |= ENETC_VF_FLAG_PF_SET_MAC;
>  	enetc_pf_set_primary_mac_addr(&priv->si->hw, vf + 1, mac);

Is it safe to use __raw_writel and __raw_writew when programming the hardware
MAC address?

Looking at enetc_pf_set_primary_mac_addr():

static void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si,
					  const u8 *addr)
{
	u32 upper = get_unaligned_le32(addr);
	u16 lower = get_unaligned_le16(addr + 4);

	__raw_writel(upper, hw->port + ENETC_PSIPMAR0(si));
	__raw_writew(lower, hw->port + ENETC_PSIPMAR1(si));
}

On big-endian architectures, these native writes lack byte-swapping, which
might result in the hardware register receiving the MAC address bytes in
reverse order compared to little-endian systems. They also lack memory
barriers, which could allow the CPU to reorder the writes.

Could this use the standard writel and writew accessors instead to ensure
proper CPU-to-device endianness conversion?

> +	mutex_unlock(&vf_state->lock);
> +
>  	return 0;
>  }

[ ... ]

> @@ -499,12 +503,15 @@ static u16 enetc_msg_pf_set_vf_primary_mac_addr(struct enetc_pf *pf,
>  		return ENETC_MSG_CMD_STATUS_FAIL;
>  	}
>  
> +	mutex_lock(&vf_state->lock);

Does the addition of this mutex widen a race condition where the VF could
modify the MAC address?

Earlier in enetc_msg_pf_set_vf_primary_mac_addr(), the addr pointer is set
to cmd->mac.sa_data, which points into the message buffer msg->vaddr.

If msg->vaddr is in coherent DMA memory accessible by the VF, the PF driver
validates the MAC address before taking the mutex, but reads it again inside
enetc_pf_set_primary_mac_addr() after taking the mutex.

Could a malicious VF exploit this double-fetch by writing an invalid MAC
address into the shared DMA buffer right after the validation check passes,
bypassing the check entirely? Might it be safer to copy the message to a
local buffer before validating it?

>  	if (vf_state->flags & ENETC_VF_FLAG_PF_SET_MAC)
>  		dev_warn(dev, "Attempt to override PF set mac addr for VF%d\n",
>  			 vf_id);
>  	else
>  		enetc_pf_set_primary_mac_addr(&pf->si->hw, vf_id + 1, addr);
>  
> +	mutex_unlock(&vf_state->lock);
> +
>  	return ENETC_MSG_CMD_STATUS_OK;
>  }

[ ... ]

> @@ -968,6 +975,9 @@ static int enetc_pf_probe(struct pci_dev *pdev,
>  					    pf->total_vfs);
>  		if (!pf->vf_state)
>  			goto err_alloc_vf_state;

If the allocation for pf->vf_state fails here, err does not appear to be
assigned a failure code before the jump.

Since err would still hold 0 from the earlier successful call to
enetc_pf_register_with_ierb(), could this cause the probe function to falsely
return success to the PCI subsystem?

If so, this might lead to a kernel panic later when enetc_pf_remove() attempts
to dereference the unallocated device structure.

> +
> +		for (int i = 0; i < pf->total_vfs; i++)
> +			mutex_init(&pf->vf_state[i].lock);

Is it necessary to call mutex_destroy() for these dynamically allocated
mutexes?

If kfree(pf->vf_state) is called during the probe error path or during
device removal, freeing the active mutex memory without destroying it first
might cause lockdep to report use-after-free or list corruption warnings
when debugging options are enabled.

>  	}
>  
>  	err = enetc_setup_mac_addresses(node, pf);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513103021.2190593-1-wei.fang@nxp.com?part=2