linux-kernel - Re: [PATCH v1] tty: Register device *after* creating the cdev for a tty

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025052801-human-aversion-3518@gregkh>
Date: Wed, 28 May 2025 10:23:39 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Max Staudt <max@...as.org>
Cc: Jiri Slaby <jirislaby@...nel.org>, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] tty: Register device *after* creating the cdev for a
 tty

On Mon, May 26, 2025 at 08:25:23PM +0900, Max Staudt wrote:
> This change makes the tty device file available only after the tty's
> backing character device is ready.
> 
> Since 6a7e6f78c235975cc14d4e141fa088afffe7062c, the class device is
> registered before the cdev is created, and userspace may pick it up,
> yet open() will fail because the backing cdev doesn't exist yet.
> Userspace is racing the bottom half of tty_register_device_attr() here,
> specifically the call to tty_cdev_add().
> 
> dev_set_uevent_suppress() was used to work around this, but this fails
> on embedded systems that rely on bare devtmpfs rather than udev.
> On such systems, the device file is created as part of device_add(),
> and userspace can pick it up via inotify, irrespective of uevent
> suppression.
> 
> So let's undo the existing patch, and create the cdev first, and only
> afterwards register the class device in the kernel's device tree.
> 
> However, this restores the original race of the cdev existing before the
> class device is registered, and an attempt to open it during this time
> will lead to tty->dev being assigned NULL by alloc_tty_struct().
> 
> alloc_tty_struct() is called via tty_init_dev() when the tty is firstly
> opened, and is entered with tty_mutex held, so let's lock the critical
> section in tty_register_device_attr() with the same global mutex.
> This guarantees that tty->dev can be assigned a sane value.

As 0-day points out, I think this adds a new locking issue :(

But it's really hard to detect this, as you are doing both a revert and
a change in the same commit.  Can you make this as 2 patches, one that
does the revert which would be "easy" to review, and the second one that
does the new fix?  That way we can detect what is going on easier.

> Fixes: 6a7e6f78c235 ("tty: close race between device register and open")
> Signed-off-by: Max Staudt <max@...as.org>

You also forgot to add cc: stable on this :(

thanks,

greg k-h