linux-kernel - Re: [PATCH 1/1] misc: Prevent double registration and deregistration of miscdevice

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: <d3d0fc0e19f939c093e6df1ff08ce23be71636a3.camel@mediatek.com>
Date: Tue, 26 Aug 2025 02:55:59 +0000
From: Xion Wang (王鑫) <Xion.Wang@...iatek.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, wsd_upstream
	<wsd_upstream@...iatek.com>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
	Huadian Liu (刘华典) <huadian.liu@...iatek.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "arnd@...db.de"
	<arnd@...db.de>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH 1/1] misc: Prevent double registration and deregistration
 of miscdevice

On Mon, 2025-08-25 at 22:28 +0200, Greg Kroah-Hartman wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Mon, Aug 25, 2025 at 04:45:47PM +0800, xion.wang@...iatek.com
> wrote:
> > From: Xion Wang <xion.wang@...iatek.com>
> > 
> > When repeated calls to misc_register() or misc_deregister() on the
> > same miscdevice could lead to kernel crashes or misc_list
> > corruption due to
> > multiple INIT_LIST_HEAD or list_del operations on the same list
> > node.
> > 
> > This patch improves the robustness of the misc device driver by
> > preventing
> > both double registration and double deregistration of miscdevice
> > instances.
> > 
> > Signed-off-by: Xion Wang <xion.wang@...iatek.com>
> > ---
> >  drivers/char/misc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> > index 558302a64dd9..2f8666312966 100644
> > --- a/drivers/char/misc.c
> > +++ b/drivers/char/misc.c
> > @@ -210,6 +210,9 @@ int misc_register(struct miscdevice *misc)
> >       int err = 0;
> >       bool is_dynamic = (misc->minor == MISC_DYNAMIC_MINOR);
> > 
> > +     if (WARN_ON(misc->this_device))
> > +             return -EEXIST;
> 
> You just crashed the kernel if this ever triggers (remember when
> panic-on-warn is set)
> 
> So please, if this can happen, properly handle it.
> 
> > +
> >       INIT_LIST_HEAD(&misc->list);
> > 
> >       mutex_lock(&misc_mtx);
> > @@ -251,6 +254,7 @@ int misc_register(struct miscdevice *misc)
> >                       misc->minor = MISC_DYNAMIC_MINOR;
> >               }
> >               err = PTR_ERR(misc->this_device);
> > +             misc->this_device = NULL;
> >               goto out;
> >       }
> > 
> > @@ -275,12 +279,13 @@ EXPORT_SYMBOL(misc_register);
> > 
> >  void misc_deregister(struct miscdevice *misc)
> >  {
> > -     if (WARN_ON(list_empty(&misc->list)))
> > +     if (WARN_ON(!misc->this_device))
> >               return;
> > 
> >       mutex_lock(&misc_mtx);
> >       list_del(&misc->list);
> >       device_destroy(&misc_class, MKDEV(MISC_MAJOR, misc->minor));
> > +     misc->this_device = NULL;
> 
> You are overloading the pointer here to mean something, please don't.
> 
> Again, why would this ever happen?  What in-tree driver does this?
> 
> thanks,
> 
> greg k-h

This issue was encountered during MTK internal stress testing,
specifically in the WiFi module on/off process. If the WiFi module
fails during the "on" process, it triggers an "off" process. However,
if the "off" process also fails, the module may not be properly
deinitialized, and the misc device may not be correctly deregistered.
On the next WiFi "on" attempt, repeated registration of the misc device
leads to corruption of the misc_list. Subsequently, when a device calls
misc_open, it may acquire the misc_lock and enter an infinite loop in
list_for_each_entry due to the corrupted list, while other threads
attempting to access their misc device nodes become blocked waiting for
the misc_lock.

This scenario exposes two issues:

Incomplete failure handling in our internal WiFi module's on/off
process (which we have already addressed internally).
The lack of a mechanism in the miscdevice framework to prevent repeated
registration or deregistration, which would improve its robustness.

During our internal code review, we found that misc_deregister() uses
list_empty to check if misc->list is empty, but this does not
effectively prevent cases where a misc_device is not registered or is
deregistered multiple times. This can result in invalid list_del
operations and trigger a kernel panic. 

Regarding your feedback, you are absolutely right. overloading the
this_device pointer as a registration state flag is not ideal. I will
revise the patch to introduce a dedicated boolean flag in struct
miscdevice to explicitly track the registration state.

We appreciate any further feedback or suggestions from the community to
help improve this solution.

thanks,

xion wang