Re: [PATCH v2 net-next 3/7] ptp: netc: export netc_timer_get_current_time() for cross-driver use

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The exported function `netc_timer_get_current_time()` assumes any non-NULL driver data belongs to the `ptp_netc` driver, leading to type confusion and memory corruption if a different driver is bound to the device.
--

--- Patch [3]: [PATCH v2 net-next 3/7] ptp: netc: export netc_timer_get_current_time() for cross-driver use ---
commit fe6f3ab81e37c2bfadc0b000bbb64a980c0130de
Author: Wei Fang <[email protected]>

ptp: netc: export netc_timer_get_current_time() for cross-driver use

This commit exports netc_timer_get_current_time() so the NETC Switch driver
can obtain the PTP time from its associated NETC Timer PCIe function. It uses
device_lock() to serialize against unbinds while reading the timer registers
via the timer driver's private data.

> diff --git a/drivers/ptp/ptp_netc.c b/drivers/ptp/ptp_netc.c
> index d33c49c86ac44..27f85818d6a8a 100644
> --- a/drivers/ptp/ptp_netc.c
> +++ b/drivers/ptp/ptp_netc.c

[ ... ]

> @@ -165,6 +165,47 @@ static u64 netc_timer_cur_time_read(struct netc_timer *priv)
>  	return netc_timer_rd64(priv, NETC_TMR_CUR_TIME_L);
>  }
>  
> +/**
> + * netc_timer_get_current_time - read the current PTP time from the NETC Timer
> + * @pdev: PCI device of the NETC Timer
> + *
> + * Reads the 64-bit current time register (TMR_CUR_TIME) from the NETC Timer
> + * device associated with @pdev. Returns 0 if the Timer driver has not yet
> + * probed or has already been removed.
> + *
> + * Context: Process context only. Acquires the device mutex via device_lock(),
> + *          which may sleep. Must not be called from atomic context, softirq,
> + *          BH, or while holding a spinlock.
> + *
> + * Return: Current PTP time in nanoseconds, or 0 if the timer is unavailable.
> + */
> +u64 netc_timer_get_current_time(struct pci_dev *pdev)
> +{
> +	struct netc_timer *priv;
> +	unsigned long flags;
> +	u64 cur_time = 0;
> +
> +	/* Serialize against driver unbind: the remove() callback runs under
> +	 * the device lock, so holding it here ensures that priv remains valid
> +	 * for the entire duration of the register read.
> +	 */
> +	device_lock(&pdev->dev);
> +
> +	priv = pci_get_drvdata(pdev);
> +	if (!priv)
> +		goto unlock_device;
> +
> +	spin_lock_irqsave(&priv->lock, flags);

[Severity: High]
If a privileged user unbinds the ptp_netc driver from this PCI device via
sysfs and binds a different driver (like vfio-pci), won't pci_get_drvdata()
return the new driver's private data structure instead of struct netc_timer?

Could this cause type confusion and memory corruption when subsequently
accessing priv->lock and priv->base during the register read?

Would it be safer to verify that pdev->driver is actually the netc_timer_driver
before dereferencing the driver data?

> +	cur_time = netc_timer_cur_time_read(priv);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +unlock_device:
> +	device_unlock(&pdev->dev);
> +
> +	return cur_time;
> +}
> +EXPORT_SYMBOL_GPL(netc_timer_get_current_time);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3