linux-kernel - Re: [PATCH v5 5/8] char: misc: Fix kunit test case miscdev_test_dynamic

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: <aHADQWaYsjK5EYsN@quatroqueijos.cascardo.eti.br>
Date: Thu, 10 Jul 2025 15:15:29 -0300
From: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	Helge Deller <deller@....de>,
	"David S. Miller" <davem@...emloft.net>,
	Andreas Larsson <andreas@...sler.com>, linux-kernel@...r.kernel.org,
	linux-parisc@...r.kernel.org, sparclinux@...r.kernel.org,
	Zijun Hu <zijun.hu@....qualcomm.com>
Subject: Re: [PATCH v5 5/8] char: misc: Fix kunit test case
 miscdev_test_dynamic_reentry() failure

On Thu, Jul 10, 2025 at 07:56:48PM +0800, Zijun Hu wrote:
> From: Zijun Hu <zijun.hu@....qualcomm.com>
> 
> misc_deregister() frees dynamic minor @misc->minor but does not
> reset it to macro MISC_DYNAMIC_MINOR, so cause kunit test case
> miscdev_test_dynamic_reentry() failure:
> 
> 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
> 
> Fix by resetting @misc->minor to MISC_DYNAMIC_MINOR in misc_deregister()
> as error handling of misc_register() does.
> 

Adding a failing test and then fixing the code does not seem the best way
to justify this change. I would rather add the fix with a proper
justification and then add the test.

On the other hand, I have found real cases where this might happen, some by
code inspection only, but I also managed to reproduce the issue here,
where:

1) wmi/dell-smbios registered minor 122, acpi_thermal_rel registered minor
123.
2) unbind "int3400 thermal" driver from its device, this will unregister
acpi_thermal_rel
3) remove dell_smbios module
4) reinstall dell_smbios module, now wmi/dell-smbios is using misc 123
5) bind the device to "int3400 thermal" driver again, acpi_thermal_rel
fails to register

I think we have a few options to fix these bugs:

1) Apply your suggested fix.
2) Fix all the buggy drivers.
3) Change API and have the minor be a misc_register parameter.

The advantage of your option is that it is simple and contained and easy to
backport.

Changing API would require changing a lot of code and hard to backport, but
I find it less error-prone than requiring the minor member to be reset, if
we end up deciding about fixing the drivers.

As for fixing individual drivers, one helpful feature is applying your
previous patch [1], but perhaps with stronger message, maybe a WARN_ON.

[1] char: misc: Disallow registering miscdevice whose minor > MISC_DYNAMIC_MINOR

I am leaning towards your suggested fix, but with different wording, and
before adding the test case.

Something like:

Some drivers may reuse the miscdevice structure after they are
deregistered. If the intention is to allocate a dynamic minor, if the minor
number is not reset to MISC_DYNAMIC_MINOR before calling misc_register, it
will try to register a previously dynamically allocated minor number, which
may have been registered by a different driver.

One such case is the acpi_thermal_rel misc device, registered by the
int3400 thermal driver. If the device is unbound from the driver and later
bound, if there was another dynamic misc device registered in between, it
would fail to register the acpi_thermal_rel misc device. Other drivers
behave similarly.

Instead of fixing all the drivers, just reset the minor member to
MISC_DYNAMIC_MINOR when calling misc_deregister in case it was a
dynamically allocated minor number.

> 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);
>  }
>  EXPORT_SYMBOL(misc_deregister);
> 
> -- 
> 2.34.1
>