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

"Wei Fang (OSS)" <[email protected]>
Newsgroups org.kernel.vger.netdev,dev.linux.lists.imx,dev.linux.lists.sashiko-reviews
Message-ID <GV2PR04MB117394A33DEB46DDA7105B67788DE2@GV2PR04MB11739.eurprd04.prod.outlook.com>
> > @@ -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?
> 

It does make sense, I will add a check for this unusual case.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.