netdev - Re: [v6,2/7] ptp: vmclock: support device notifications

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f663dbfff1568d8924a2a3b8fbd6532b97f54b68.camel@infradead.org>
Date: Fri, 23 Jan 2026 12:16:46 -0800
From: David Woodhouse <dwmw2@...radead.org>
To: Jakub Kicinski <kuba@...nel.org>, itazur@...zon.com
Cc: graf@...zon.de, itazur@...zon.co.uk, conor+dt@...nel.org, 
 devicetree@...r.kernel.org, xmarcalx@...zon.co.uk,
 linux-kernel@...r.kernel.org,  edumazet@...gle.com, robh@...nel.org,
 richardcochran@...il.com,  andrew+netdev@...n.ch, mzxreary@...inter.de,
 pabeni@...hat.com,  netdev@...r.kernel.org, krzk+dt@...nel.org,
 davem@...emloft.net, mail@...alios.io
Subject: Re: [v6,2/7] ptp: vmclock: support device notifications

On Thu, 2026-01-22 at 19:11 -0800, Jakub Kicinski wrote:
> 
> > +	status = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY,
> > +					     vmclock_acpi_notification_handler,
> > +					     dev);
> 
> Since acpi_install_notify_handler() is called here, should there be a
> corresponding acpi_remove_notify_handler() in vmclock_remove()? Currently
> the handler remains registered after device removal, which could lead to
> a use-after-free if the hypervisor sends a notification after the driver
> state has been freed.
> 
> [ ... ]
> 
> > @@ -549,6 +653,11 @@ static int vmclock_probe(struct platform_device *pdev)
> >   	if (ret)
> >   		return ret;
> > 
> > +	init_waitqueue_head(&st->disrupt_wait);
> > +	ret = vmclock_setup_notification(dev, st);
> > +	if (ret)
> > +		return ret;
> 
> [ ... ]
> 
> > @@ -581,6 +690,8 @@ static int vmclock_probe(struct platform_device *pdev)
> >   		return -ENODEV;
> >   	}
> > 
> > +	dev->driver_data = st;
> 
> Is there a race window here? The ACPI notification handler is installed
> in vmclock_setup_notification() which runs earlier in vmclock_probe(),
> but the handler accesses device->driver_data. If the hypervisor sends a
> notification between vmclock_setup_notification() and this assignment,
> vmclock_acpi_notification_handler() would dereference NULL or garbage.
> 
> I see this is fixed later in the series by commit 47fdd294385e which
> moves dev->driver_data = st before vmclock_setup_notification().

With the incremental diff below, I've fixed both of these in
https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/vmclock

Takahiro and Babis have the test environment set up to actually
exercise the notifications, so I'll let one of them actually do the
testing and repost v7.

Takahiro, please remember to add your own 'Signed-off-by:' as you do.

Thanks!


--- a/drivers/ptp/ptp_vmclock.c
+++ b/drivers/ptp/ptp_vmclock.c
@@ -490,17 +490,6 @@ static const struct file_operations vmclock_miscdev_fops = {
 
 /* module operations */
 
-static void vmclock_remove(void *data)
-{
-	struct vmclock_state *st = data;
-
-	if (st->ptp_clock)
-		ptp_clock_unregister(st->ptp_clock);
-
-	if (st->miscdev.minor != MISC_DYNAMIC_MINOR)
-		misc_deregister(&st->miscdev);
-}
-
 #if IS_ENABLED(CONFIG_ACPI)
 static acpi_status vmclock_acpi_resources(struct acpi_resource *ares, void *data)
 {
@@ -636,6 +625,32 @@ static int vmclock_setup_notification(struct device *dev,
 	return vmclock_setup_of_notification(dev);
 }
 
+static void vmclock_remove(void *data)
+{
+	struct device *dev = data;
+	struct vmclock_state *st = dev->driver_data;
+
+	if (!st) {
+		dev_err(dev, "vmclock_remove() called with NULL driver_data");
+		return;
+	}
+
+#if IS_ENABLED(CONFIG_ACPI)
+	if (has_acpi_companion(dev))
+		acpi_remove_notify_handler(ACPI_COMPANION(dev)->handle,
+					   ACPI_DEVICE_NOTIFY,
+					   vmclock_acpi_notification_handler);
+#endif
+
+	if (st->ptp_clock)
+		ptp_clock_unregister(st->ptp_clock);
+
+	if (st->miscdev.minor != MISC_DYNAMIC_MINOR)
+		misc_deregister(&st->miscdev);
+
+	dev->driver_data = NULL;
+}
+
 static void vmclock_put_idx(void *data)
 {
 	struct vmclock_state *st = data;
@@ -701,12 +716,13 @@ static int vmclock_probe(struct platform_device *pdev)
 
 	st->miscdev.minor = MISC_DYNAMIC_MINOR;
 
-	ret = devm_add_action_or_reset(&pdev->dev, vmclock_remove, st);
+	init_waitqueue_head(&st->disrupt_wait);
+	dev->driver_data = st;
+
+	ret = devm_add_action_or_reset(&pdev->dev, vmclock_remove, dev);
 	if (ret)
 		return ret;
 
-	init_waitqueue_head(&st->disrupt_wait);
-	dev->driver_data = st;
 	ret = vmclock_setup_notification(dev, st);
 	if (ret)
 		return ret;



Download attachment "smime.p7s" of type "application/pkcs7-signature" (5069 bytes)