linux-kernel - Re: [PATCH 3/4] char: misc: restrict the dynamic range to exclude reserved minors

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: <2025022010-next-engaging-5467@gregkh>
Date: Thu, 20 Feb 2025 16:05:39 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
Cc: linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dirk VanDerMerwe <dirk.vandermerwe@...hos.com>,
	Vimal Agrawal <vimal.agrawal@...hos.com>, kernel-dev@...lia.com
Subject: Re: [PATCH 3/4] char: misc: restrict the dynamic range to exclude
 reserved minors

On Thu, Feb 20, 2025 at 11:52:31AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Thu, Feb 20, 2025 at 03:31:11PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 23, 2025 at 09:32:48AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > > When this was first reported [1], the possibility of having sufficient
> > > number of dynamic misc devices was theoretical.
> > > 
> > > What we know from commit ab760791c0cf ("char: misc: Increase the maximum
> > > number of dynamic misc devices to 1048448"), is that the miscdevice
> > > interface has been used for allocating more than the single-shot devices it
> > > was designed for.
> > 
> > Do we have any in-kernel drivers that abuse it this way?  If so, let's
> > fix them up.
> > 
> 
> >From the discussion 15 years ago, though found only by code inspection, dlm
> was theoretically capable of creating such multiple devices. But, in
> practice, its user space never did create more than one. 
> 
> But from commit ab760791c0cf ("char: misc: Increase the maximum number of
> dynamic misc devices to 1048448") description, we know at least
> coresight_tmc is abusing it like that.

Ick.

> I can work on fixing coresight_tmc and any other abusers, but they will
> require their own class (though I thought about making it possible to
> create compatibility symlinks under /sys/class/misc/). So, this should take
> a bit longer. In the meantime, I think we shold apply something like this
> patch for v6.15 and even consider it for stable.

No such need for compatibility symlinks, devices should be able to be
found in the correct location no matter where they are in the system,
right?

But yes, a class is needed, thanks!

> > > On such systems, it is certain that the dynamic allocation will allocate
> > > certain reserved minor numbers, leading to failures when a later driver
> > > tries to claim its reserved number.
> > > 
> > > Fixing this is a simple matter of defining the IDA range to allocate from
> > > to exclude minors up to and including 15.
> > > 
> > > [1] https://lore.kernel.org/all/1257813017-28598-3-git-send-email-cascardo@holoscopio.com/
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
> > > ---
> > >  drivers/char/misc.c        | 4 +++-
> > >  include/linux/miscdevice.h | 1 +
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> > > index 2cf595d2e10b..7a768775e558 100644
> > > --- a/drivers/char/misc.c
> > > +++ b/drivers/char/misc.c
> > > @@ -68,8 +68,10 @@ static int misc_minor_alloc(int minor)
> > >  	int ret = 0;
> > >  
> > >  	if (minor == MISC_DYNAMIC_MINOR) {
> > > +		int max = DYNAMIC_MINORS - 1 - MISC_STATIC_MAX_MINOR - 1;
> > >  		/* allocate free id */
> > > -		ret = ida_alloc_max(&misc_minors_ida, DYNAMIC_MINORS - 1, GFP_KERNEL);
> > > +		/* Minors from 0 to 15 are reserved. */
> > > +		ret = ida_alloc_max(&misc_minors_ida, max, GFP_KERNEL);
> > >  		if (ret >= 0) {
> > >  			ret = DYNAMIC_MINORS - ret - 1;
> > >  		} else {
> > > diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
> > > index 69e110c2b86a..911a294d17b5 100644
> > > --- a/include/linux/miscdevice.h
> > > +++ b/include/linux/miscdevice.h
> > > @@ -21,6 +21,7 @@
> > >  #define APOLLO_MOUSE_MINOR	7	/* unused */
> > >  #define PC110PAD_MINOR		9	/* unused */
> > >  /*#define ADB_MOUSE_MINOR	10	FIXME OBSOLETE */
> > > +#define MISC_STATIC_MAX_MINOR	15	/* Top of first reserved range */
> > 
> > I don't understand, why is 15 the magic number here?  All of those
> > "unused" values can just be removed, all systems should be using dynamic
> > /dev/ now for many many years, and even if they aren't, these minors
> > aren't being used by anyone else as the in-kernel users are long gone.
> > 
> > So why are we reserving this range if no one needs it?
> 
> Because those were reserved historically. They are still documented under
> Documentation/admin-guide/devices.txt. What do we gain by not reserving
> those? Since we moved past 255 for dynamic allocation, we are not going to
> miss those 15 minors.

True, but it turns out we don't need to reserve them anymore, so let's
not.

> One alternative is that we just disregard the 0-255 range entirely for
> dynamic allocation, which should also simplify the code a lot. If that
> would be preferable, I can work on that and submit it soon.

I'm all for that, that might just make things much simpler, as a dynamic
number shouldn't care what it's set at, right?  Try it and see?

thanks,

greg k-h