Re: [PATCH v3 net-next 13/14] net: enetc: use alloc_etherdev_mqs() to create netdev for VF driver

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware-provided values for TX/RX rings are not bounded against internal fixed-size arrays, leading to out-of-bounds memory writes.
- [High] Missing `cancel_work_sync` for `tx_onestep_tstamp` leads to a Use-After-Free when the device is removed.
--

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

net: enetc: use alloc_etherdev_mqs() to create netdev for VF driver
    
The VF driver uses alloc_etherdev_mq() with ENETC_MAX_NUM_TXQS as the
queue count, which forces the TX and RX queue counts to be equal and
uses a compile-time constant rather than the actual hardware capability.
This patch switches to alloc_etherdev_mqs() so that the TX and RX queue
counts are set independently based on the actual rings assigned.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 9cdb0a4d6bafb..7dcb4a0246f52 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> @@ -317,7 +317,14 @@ static int enetc_vf_probe(struct pci_dev *pdev,
>  
>  	enetc_get_si_caps(si);
>  

[Severity: High]
This isn't a bug introduced by this patch, but can a malicious PF or
hypervisor cause an out-of-bounds write here?

Looking at enetc_get_si_caps(), it reads si->num_tx_rings and
si->num_rx_rings directly from the hardware register ENETC_SICAPR0 and
masks them with 0xff. This allows values up to 255.

While this patch properly bounds the values passed to alloc_etherdev_mqs()
using min(), the driver's internal arrays priv->tx_ring (size 16) and
priv->int_vector (size ENETC_MAX_BDR_INT) are still exposed to these
unbounded hardware values. During initialization, enetc_int_vector_init()
writes to these arrays using the unbounded ring counts, which could
overflow the fixed-size arrays.

Should the hardware capabilities be explicitly bounded against the internal
array sizes when they are read in enetc_get_si_caps()?

> -	ndev = alloc_etherdev_mq(sizeof(*priv), ENETC_MAX_NUM_TXQS);
> +	/* Currently, the supported SoCs have a max of 6 CPUs and the VFs
> +	 * have less than 6 RX/TX rings. So no issues for these supported
> +	 * SoCs, but for future SoCs which have more CPUs or more TX/RX
> +	 * rings, all the related logic needs to be improved.
> +	 */
> +	ndev = alloc_etherdev_mqs(sizeof(*priv),
> +				  min(si->num_tx_rings, ENETC_MAX_NUM_TXQS),
> +				  min(si->num_rx_rings, ENETC_MAX_NUM_TXQS));
>  	if (!ndev) {
>  		err = -ENOMEM;
>  		dev_err(&pdev->dev, "netdev creation failed\n");

[ ... ]

>  err_alloc_si_res:
>  	si->ops->teardown_cbdr(si);
>  err_setup_cbdr:
>  	si->ndev = NULL;
>  	free_netdev(ndev);

[Severity: High]
This is a pre-existing issue, but does this driver have a use-after-free
risk during device removal?

In the device removal path enetc_vf_remove(), which similarly calls
free_netdev(), the driver unregisters and frees the netdev (which also
frees the priv structure) without first calling
cancel_work_sync(&priv->tx_onestep_tstamp).

If a user requests hardware timestamping for outgoing packets and the
device is concurrently unbound, could the background workqueue thread
execute enetc_tx_onestep_tstamp() and dereference the freed priv pointer?

>  err_alloc_netdev:
>  err_get_driver_data:
>  	msg = si->msg;
>  	enetc_pci_remove(pdev);
>  	enetc_msg_dma_free(&pdev->dev, &msg);
>  
>  	return err;
>  }

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