[PATCH] posix-clock: bind the device before the cdev is published

[PATCH] posix-clock: bind the device before the cdev is published

[fangsz]

posix_clock_register() adds the character device and only afterwards
stores the device pointer:

	cdev_init(&clk->cdev, &posix_clock_file_operations);
	err = cdev_device_add(&clk->cdev, dev);
	if (err)
		...
	clk->dev = dev;

device_add() publishes the /dev/ptpN node through devtmpfs_create_node()
before it returns, so a concurrent open() can reach posix_clock_open()
while clk->dev is still NULL. get_device(NULL) is a no-op, so such a file
does not hold the reference it is supposed to hold, while
posix_clock_release() unconditionally calls put_device(clk->dev) and thus
drops a reference that was never taken.

That stray put frees the parent device early. When the character device
is released later, cdev_default_release() calls kobject_put() on the
already freed device and the refcount saturates:

  refcount_t: underflow; use-after-free.
  WARNING: CPU: 6 PID: 4270 at lib/refcount.c:28 refcount_warn_saturate+0xec/0x140
  ...
  Call trace:
   refcount_warn_saturate+0xec/0x140
   kobject_put+0x160/0x208
   cdev_default_release+0x78/0x90
   kobject_put+0xa8/0x208
   cdev_put+0x20/0x40
   __fput+0x2cc/0x2e8
   __arm64_sys_close+0x3c/0x88

The window is small, but it is hit in practice once a driver registers
and unregisters the PTP clock repeatedly, for example on every link up
and down, while ptp4l keeps reopening the node:

  [   50.048138] eth0: registered PTP clock
  [   50.767426] ptp4l: refcount_t: underflow; use-after-free.
  [   55.735110] eth0: Link is Down
  [   55.816736] eth0: registered PTP clock

The ordering was introduced by a33121e5487b, which added the
get_device()/put_device() pair around clk->dev but left the assignment
at the end of posix_clock_register(). Bind clk->dev before the character
device is published and clear it again when cdev_device_add() fails, so
that an open() racing with the registration always finds a valid device
to take a reference on.

Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and cdev")
Cc: stable@vger.kernel.org
Signed-off-by: fangsz <315539811@qq.com>
---
 kernel/time/posix-clock.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c
index dab3729..1c61382 100644
--- a/kernel/time/posix-clock.c
+++ b/kernel/time/posix-clock.c
@@ -163,15 +163,23 @@ int posix_clock_register(struct posix_clock *clk, struct device *dev)
 
 	init_rwsem(&clk->rwsem);
 
+	/*
+	 * cdev_device_add() publishes the device node before it returns, so
+	 * clk->dev must be set up front.  Otherwise an open() racing with the
+	 * registration skips the get_device() in posix_clock_open() while
+	 * posix_clock_release() still drops a reference for it.
+	 */
+	clk->dev = dev;
+
 	cdev_init(&clk->cdev, &posix_clock_file_operations);
 	err = cdev_device_add(&clk->cdev, dev);
 	if (err) {
+		clk->dev = NULL;
 		pr_err("%s unable to add device %d:%d\n",
 			dev_name(dev), MAJOR(dev->devt), MINOR(dev->devt));
 		return err;
 	}
 	clk->cdev.owner = clk->ops.owner;
-	clk->dev = dev;
 
 	return 0;
 }

Re: [PATCH] posix-clock: bind the device before the cdev is published

[Frederic Weisbecker]

Le Tue, Oct 27, 2026 at 01:18:32AM +0800, fangsz a écrit :
> posix_clock_register() adds the character device and only afterwards
> stores the device pointer:
> 
> 	cdev_init(&clk->cdev, &posix_clock_file_operations);
> 	err = cdev_device_add(&clk->cdev, dev);
> 	if (err)
> 		...
> 	clk->dev = dev;
> 
> device_add() publishes the /dev/ptpN node through devtmpfs_create_node()
> before it returns, so a concurrent open() can reach posix_clock_open()
> while clk->dev is still NULL. get_device(NULL) is a no-op, so such a file
> does not hold the reference it is supposed to hold, while
> posix_clock_release() unconditionally calls put_device(clk->dev) and thus
> drops a reference that was never taken.
> 
> That stray put frees the parent device early. When the character device
> is released later, cdev_default_release() calls kobject_put() on the
> already freed device and the refcount saturates:
> 
>   refcount_t: underflow; use-after-free.
>   WARNING: CPU: 6 PID: 4270 at lib/refcount.c:28 refcount_warn_saturate+0xec/0x140
>   ...
>   Call trace:
>    refcount_warn_saturate+0xec/0x140
>    kobject_put+0x160/0x208
>    cdev_default_release+0x78/0x90
>    kobject_put+0xa8/0x208
>    cdev_put+0x20/0x40
>    __fput+0x2cc/0x2e8
>    __arm64_sys_close+0x3c/0x88
> 
> The window is small, but it is hit in practice once a driver registers
> and unregisters the PTP clock repeatedly, for example on every link up
> and down, while ptp4l keeps reopening the node:
> 
>   [   50.048138] eth0: registered PTP clock
>   [   50.767426] ptp4l: refcount_t: underflow; use-after-free.
>   [   55.735110] eth0: Link is Down
>   [   55.816736] eth0: registered PTP clock
> 
> The ordering was introduced by a33121e5487b, which added the
> get_device()/put_device() pair around clk->dev but left the assignment
> at the end of posix_clock_register(). Bind clk->dev before the character
> device is published and clear it again when cdev_device_add() fails, so
> that an open() racing with the registration always finds a valid device
> to take a reference on.
> 
> Fixes: a33121e5487b ("ptp: fix the race between the release of ptp_clock and cdev")
> Cc: stable@vger.kernel.org
> Signed-off-by: fangsz <315539811@qq.com>

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

-- 
Frederic Weisbecker
SUSE Labs