linux-kernel - Re: [PATCH v4 5/8] char: misc: Make registering miscdevice reentry who wants dynamic minor

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: <2025070948-habitable-figment-d1aa@gregkh>
Date: Wed, 9 Jul 2025 15:10:26 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Arnd Bergmann <arnd@...db.de>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	Helge Deller <deller@....de>,
	"David S. Miller" <davem@...emloft.net>,
	Andreas Larsson <andreas@...sler.com>,
	Thadeu Lima de Souza Cascardo <cascardo@...lia.com>,
	linux-kernel@...r.kernel.org, linux-parisc@...r.kernel.org,
	sparclinux@...r.kernel.org, Zijun Hu <zijun.hu@....qualcomm.com>
Subject: Re: [PATCH v4 5/8] char: misc: Make registering miscdevice reentry
 who wants dynamic minor

On Wed, Jul 09, 2025 at 08:41:36PM +0800, Zijun Hu wrote:
> On 2025/7/6 16:55, Greg Kroah-Hartman wrote:
> >> | Invalid fixed minor 257 for miscdevice 'miscdyn_a'
> >> | #miscdev_test_dynamic_reentry: ASSERTION FAILED at misc_minor_kunit.c:639
> >> | Expected ret == 0, but
> >> | ret == -22 (0xffffffffffffffea)
> >> | [FAILED] miscdev_test_dynamic_reentry
> >>
> >> misc_register()/misc_deregister() are sometimes invoked by driver's
> >> probe()/remove() separately, which has reentry requirement.
> > What do you mean?  Why is it required that this is reentrant?  What
> > in-kernel drivers require this?
> > 
> 
> miscdevice APIs are sometimes used by the following way, "drv_probe()->
> drv_remove()->drv_probe()" is possible, so "misc_register()->
> misc_deregister()->misc_register()" is possible and considered by
> previous patch's test case, which needs to register misc_dev again
> without any reinitialization, namely, reentry.
> 
> actually, several in-kernel codes have such usages.
> 
> static struct miscdevice misc_dev = {
> 	...
> 		.minor = MISC_DYNAMIC_MINOR,
> 	...
> };
> 
> int drv_probe() {
> ...
>     // static misc_dev is not initialized before registering.
> 	misc_register(&misc_dev);
> ...
> }
> 
> void drv_remove() {
> 	...
> 	misc_deregister(&misc_dev);
> 	...
> }
> 
> struct device_driver driver = {
> 	.probe = drv_probe,
> 	.remove = drv_remove,
> };
> 
> >> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
> >> as error handling of misc_register() does.
> >>
> >> Signed-off-by: Zijun Hu <zijun.hu@....qualcomm.com>
> >> ---
> >>  drivers/char/misc.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> >> index b8e66466184fa21fb66d968db7950e0b5669ac43..96ed343cf5c8509a09855020049a9af82a3ede95 100644
> >> --- a/drivers/char/misc.c
> >> +++ b/drivers/char/misc.c
> >> @@ -288,6 +288,8 @@ void misc_deregister(struct miscdevice *misc)
> >>  	list_del(&misc->list);
> >>  	device_destroy(&misc_class, MKDEV(MISC_MAJOR, misc->minor));
> >>  	misc_minor_free(misc->minor);
> >> +	if (misc->minor > MISC_DYNAMIC_MINOR)
> >> +		misc->minor = MISC_DYNAMIC_MINOR;
> >>  	mutex_unlock(&misc_mtx);
> > misc is being unregistered here, so why are you changing the minor
> > field?  It's now invalid as it is not registered, so this value should
> > never be relied on at all, neither is anything else in this structure.
> 
> reset its minor code is to register it again without re-initialization
> successfully
> 
> the other members are handed by misc_deregister() properly and don't
> effect re-registering.

Then we are getting lucky, let's clear out all the fields we modified
and later rely on as this feels very fragile.

thanks,

greg k-h