* [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
@ 2026-07-29 12:27 Pei Xiao
2026-07-29 12:48 ` sashiko-bot
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Pei Xiao @ 2026-07-29 12:27 UTC (permalink / raw)
To: dlemoal, cassel, linux, axboe, linux-ide, linux-kernel
Cc: shuangpeng.kernel, Pei Xiao
The function new_device_store() calls driver_find() without any
protection against concurrent driver unregistration. This can lead
to a use-after-free (UAF) when a driver is unloaded (via rmmod)
in parallel with a new device addition via sysfs.
The race window exists because driver_find() returns a pointer to
the driver's private data, but does not increase its reference
count. The caller is responsible for ensuring the driver remains
valid, but new_device_store() did not hold any lock or reference
during the lookup and subsequent use.
Concurrently, pata_parport_unregister_driver() releases the
pi_mutex before calling driver_unregister(), allowing new_device_store
to proceed with a stale pointer after the driver has been freed.
Fix this by expanding the critical section protected by pi_mutex
in both functions:
- In new_device_store(): acquire pi_mutex before calling driver_find(),
and keep it held until all uses of the found driver pointer are
completed.
- In pata_parport_unregister_driver(): hold pi_mutex throughout the
entire unregistration process, including the driver_unregister()
call, so that no concurrent lookup can see a partially removed
driver.
Thread A (new_device_store) | Thread B (pata_parport_unregister_driver)
driver_find("aten") |
| mutex_lock(&pi_mutex)
| idr_remove(&protocols, id)
| mutex_unlock(&pi_mutex)
| driver_unregister(&pr->driver)
| (frees driver_private)
/* continues using stale driv */|
-> UAF! |
Logs:
BUG: KASAN: slab-use-after-free in driver_find+0xd0/0xd4
Read of size 8 at addr ffffff9f970dc690 by task sh/4737
Call trace:
show_stack+0x14/0x1c (C)
dump_stack_lvl+0x70/0x84
print_report+0xf4/0x5a4
kasan_report+0xa0/0xe4
__asan_report_load8_noabort+0x18/0x20
driver_find+0xd0/0xd4
new_device_store+0x140/0x30c [pata_parport]
bus_attr_store+0x5c/0x94
sysfs_kf_write+0x1c4/0x25c
...
Allocated by task 4734:
kasan_save_stack+0x28/0x4c
kasan_save_track+0x1c/0x34
kasan_save_alloc_info+0x3c/0x4c
__kasan_kmalloc+0x98/0xac
__kmalloc_cache_noprof+0x158/0x3cc
bus_add_driver+0x70/0x4d8
driver_register+0xf0/0x3b0
pata_parport_register_driver+0xb8/0x1c4 [pata_parport]
0xffffffc07f5ce014
do_one_initcall+0xb8/0x36c
do_init_module+0x230/0x6d4
...
Freed by task 4737:
kasan_save_stack+0x28/0x4c
kasan_save_track+0x1c/0x34
kasan_save_free_info+0x48/0x8c
__kasan_slab_free+0x5c/0x84
kfree+0x174/0x3e0
driver_release+0x1c/0x7c
kobject_put+0x178/0x47c
driver_find+0x78/0xd4
new_device_store+0x140/0x30c [pata_parport]
bus_attr_store+0x5c/0x94
sysfs_kf_write+0x1c4/0x25c
...
Fixes: 72f2b0b21850 ("drivers/block: Move PARIDE protocol modules to drivers/ata/pata_parport")
Reported-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Closes: https://lore.kernel.org/lkml/20260728024015.2014674-1-shuangpeng.kernel@gmail.com/
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
drivers/ata/pata_parport/pata_parport.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 40baeac594a9..34f09192a419 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -3,6 +3,7 @@
* Copyright 2023 Ondrej Zary
* based on paride.c by Grant R. Guenther <grant@torque.net>
*/
+#include <linux/cleanup.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/parport.h>
@@ -618,8 +619,9 @@ void pata_parport_unregister_driver(struct pi_protocol *pr)
break;
}
idr_remove(&protocols, id);
- mutex_unlock(&pi_mutex);
driver_unregister(&pr->driver);
+ mutex_unlock(&pi_mutex);
+
}
EXPORT_SYMBOL_GPL(pata_parport_unregister_driver);
@@ -646,6 +648,7 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
port_wanted = -1;
}
+ guard(mutex)(&pi_mutex);
drv = driver_find(protocol, &pata_parport_bus_type);
if (!drv) {
if (strcmp(protocol, "auto")) {
@@ -656,15 +659,12 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
} else {
pr_wanted = container_of(drv, struct pi_protocol, driver);
}
-
- mutex_lock(&pi_mutex);
/* walk all parports */
idr_for_each_entry(&parport_list, parport, port_num) {
if (port_num == port_wanted || port_wanted == -1) {
parport = parport_find_number(port_num);
if (!parport) {
pr_err("no such port %s\n", port);
- mutex_unlock(&pi_mutex);
return -ENODEV;
}
/* walk all protocols */
@@ -677,7 +677,6 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
parport_put_port(parport);
}
}
- mutex_unlock(&pi_mutex);
if (!ok)
return -ENODEV;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
2026-07-29 12:27 [PATCH] ata: pata_parport: Fix use-after-free in new_device_store Pei Xiao
@ 2026-07-29 12:48 ` sashiko-bot
2026-07-29 13:10 ` Niklas Cassel
2026-07-29 23:42 ` Damien Le Moal
2 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-07-29 12:48 UTC (permalink / raw)
To: Pei Xiao; +Cc: linux-ide, cassel, dlemoal
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Out-of-bounds array read due to lack of lower bound checking on user-provided `mode`.
- [High] Kernel soft lockup (infinite busy-wait) due to negative `delay` values provided via sysfs.
--
--- Patch [1]: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store ---
commit 8244d117e0d1faf887a5bd4b26650cebdd4ecedb
Author: Pei Xiao <xiaopei01@kylinos.cn>
ata: pata_parport: Fix use-after-free in new_device_store
This commit fixes a use-after-free vulnerability in new_device_store() by
expanding the critical section protected by pi_mutex. It uses guard(mutex) to
ensure the driver pointer remains valid during lookup and subsequent uses,
preventing races with concurrent unregistration.
Closes: https://lore.kernel.org/lkml/20260728024015.2014674-1-shuangpeng.kernel@gmail.com/
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 40baeac594a9f..34f09192a4190 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
[ ... ]
> @@ -646,6 +648,7 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
> port_wanted = -1;
> }
>
[Severity: High]
This is a pre-existing issue, but does this sysfs input parsing properly
validate negative values for the mode parameter?
When new_device_store() uses sscanf() to read user input earlier in this
function, mode is parsed as a signed integer. If a user provides a negative
mode (e.g., -2), it bypasses the bounds check in pi_probe_mode(), which
uses a signed comparison:
drivers/ata/pata_parport/pata_parport.c:pi_probe_mode() {
...
if (pi->mode >= max)
return false;
...
}
This negative mode can then be passed to protocol drivers and used as an array
index, which could result in an out-of-bounds memory read. For example, in
bpck6_open():
drivers/ata/pata_parport/bpck6.c:bpck6_open() {
...
i = mode_map[pi->mode] & 0x0C;
...
}
Could a lower bound check be added for the mode parameter?
[Severity: High]
This is also a pre-existing issue, but does this function properly handle
negative values for the delay parameter?
Similar to the mode parameter, delay is parsed via sscanf() as a signed
integer without lower bounds checking. The delay is heavily used in the
subsystem via the delay_p macro:
drivers/ata/pata_parport/pata_parport.h:
#define delay_p (pi->delay ? udelay(pi->delay) : (void)0)
Since udelay() takes an unsigned long, passing a negative delay will
implicitly cast it to a massive positive number (e.g., ULONG_MAX - 1). This
will cause udelay() to busy-wait for an effectively infinite amount of time,
resulting in a CPU soft lockup.
Could the delay parameter be validated to prevent it from being negative?
> + guard(mutex)(&pi_mutex);
> drv = driver_find(protocol, &pata_parport_bus_type);
> if (!drv) {
> if (strcmp(protocol, "auto")) {
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/7a9734be161cd051063ccc3bdeb377c55a83c4bf.1785327915.git.xiaopei01@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
2026-07-29 12:27 [PATCH] ata: pata_parport: Fix use-after-free in new_device_store Pei Xiao
2026-07-29 12:48 ` sashiko-bot
@ 2026-07-29 13:10 ` Niklas Cassel
2026-07-30 1:47 ` Pei Xiao
2026-07-29 23:42 ` Damien Le Moal
2 siblings, 1 reply; 6+ messages in thread
From: Niklas Cassel @ 2026-07-29 13:10 UTC (permalink / raw)
To: Pei Xiao
Cc: dlemoal, linux, axboe, linux-ide, linux-kernel, shuangpeng.kernel
On Wed, Jul 29, 2026 at 08:27:53PM +0800, Pei Xiao wrote:
(snip)
> Fixes: 72f2b0b21850 ("drivers/block: Move PARIDE protocol modules to drivers/ata/pata_parport")
This does not look like the correct Fixes tag, since it is simply moving
files.
Not saying that it is super important to have a Fixes tag.
Without having looked, I'm guessing that there is a chance that this has
been racy/broken since the driver was introduced.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
2026-07-29 13:10 ` Niklas Cassel
@ 2026-07-30 1:47 ` Pei Xiao
0 siblings, 0 replies; 6+ messages in thread
From: Pei Xiao @ 2026-07-30 1:47 UTC (permalink / raw)
To: Niklas Cassel
Cc: dlemoal, linux, axboe, linux-ide, linux-kernel, shuangpeng.kernel
在 2026/7/29 21:10, Niklas Cassel 写道:
> On Wed, Jul 29, 2026 at 08:27:53PM +0800, Pei Xiao wrote:
>
> (snip)
>
>> Fixes: 72f2b0b21850 ("drivers/block: Move PARIDE protocol modules to drivers/ata/pata_parport")
>
> This does not look like the correct Fixes tag, since it is simply moving
> files.
>
Thank you for your reply. Perhaps I didn’t know how to use the --follow
option correctly, so the oldest commit I saw was the rename commit. But
now I’ve learned to use --follow, and I found the commit:
Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
> Not saying that it is super important to have a Fixes tag.
>
> Without having looked, I'm guessing that there is a chance that this has
> been racy/broken since the driver was introduced.
>
>
> Kind regards,
> Niklas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
2026-07-29 12:27 [PATCH] ata: pata_parport: Fix use-after-free in new_device_store Pei Xiao
2026-07-29 12:48 ` sashiko-bot
2026-07-29 13:10 ` Niklas Cassel
@ 2026-07-29 23:42 ` Damien Le Moal
2026-07-30 2:58 ` Pei Xiao
2 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2026-07-29 23:42 UTC (permalink / raw)
To: Pei Xiao, cassel, linux, axboe, linux-ide, linux-kernel; +Cc: shuangpeng.kernel
On 7/29/26 21:27, Pei Xiao wrote:
> The function new_device_store() calls driver_find() without any
> protection against concurrent driver unregistration. This can lead
> to a use-after-free (UAF) when a driver is unloaded (via rmmod)
> in parallel with a new device addition via sysfs.
>
> The race window exists because driver_find() returns a pointer to
> the driver's private data, but does not increase its reference
> count. The caller is responsible for ensuring the driver remains
> valid, but new_device_store() did not hold any lock or reference
> during the lookup and subsequent use.
>
> Concurrently, pata_parport_unregister_driver() releases the
> pi_mutex before calling driver_unregister(), allowing new_device_store
> to proceed with a stale pointer after the driver has been freed.
>
> Fix this by expanding the critical section protected by pi_mutex
> in both functions:
>
> - In new_device_store(): acquire pi_mutex before calling driver_find(),
> and keep it held until all uses of the found driver pointer are
> completed.
> - In pata_parport_unregister_driver(): hold pi_mutex throughout the
> entire unregistration process, including the driver_unregister()
> call, so that no concurrent lookup can see a partially removed
> driver.
>
> Thread A (new_device_store) | Thread B (pata_parport_unregister_driver)
> driver_find("aten") |
> | mutex_lock(&pi_mutex)
> | idr_remove(&protocols, id)
> | mutex_unlock(&pi_mutex)
> | driver_unregister(&pr->driver)
> | (frees driver_private)
> /* continues using stale driv */|
> -> UAF! |
>
> Logs:
> BUG: KASAN: slab-use-after-free in driver_find+0xd0/0xd4
> Read of size 8 at addr ffffff9f970dc690 by task sh/4737
> Call trace:
> show_stack+0x14/0x1c (C)
> dump_stack_lvl+0x70/0x84
> print_report+0xf4/0x5a4
> kasan_report+0xa0/0xe4
> __asan_report_load8_noabort+0x18/0x20
> driver_find+0xd0/0xd4
> new_device_store+0x140/0x30c [pata_parport]
> bus_attr_store+0x5c/0x94
> sysfs_kf_write+0x1c4/0x25c
> ...
>
> Allocated by task 4734:
> kasan_save_stack+0x28/0x4c
> kasan_save_track+0x1c/0x34
> kasan_save_alloc_info+0x3c/0x4c
> __kasan_kmalloc+0x98/0xac
> __kmalloc_cache_noprof+0x158/0x3cc
> bus_add_driver+0x70/0x4d8
> driver_register+0xf0/0x3b0
> pata_parport_register_driver+0xb8/0x1c4 [pata_parport]
> 0xffffffc07f5ce014
> do_one_initcall+0xb8/0x36c
> do_init_module+0x230/0x6d4
> ...
>
> Freed by task 4737:
> kasan_save_stack+0x28/0x4c
> kasan_save_track+0x1c/0x34
> kasan_save_free_info+0x48/0x8c
> __kasan_slab_free+0x5c/0x84
> kfree+0x174/0x3e0
> driver_release+0x1c/0x7c
> kobject_put+0x178/0x47c
> driver_find+0x78/0xd4
> new_device_store+0x140/0x30c [pata_parport]
> bus_attr_store+0x5c/0x94
> sysfs_kf_write+0x1c4/0x25c
> ...
>
> Fixes: 72f2b0b21850 ("drivers/block: Move PARIDE protocol modules to drivers/ata/pata_parport")
> Reported-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> Closes: https://lore.kernel.org/lkml/20260728024015.2014674-1-shuangpeng.kernel@gmail.com/
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
> drivers/ata/pata_parport/pata_parport.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 40baeac594a9..34f09192a419 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -3,6 +3,7 @@
> * Copyright 2023 Ondrej Zary
> * based on paride.c by Grant R. Guenther <grant@torque.net>
> */
> +#include <linux/cleanup.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/parport.h>
> @@ -618,8 +619,9 @@ void pata_parport_unregister_driver(struct pi_protocol *pr)
> break;
> }
> idr_remove(&protocols, id);
> - mutex_unlock(&pi_mutex);
> driver_unregister(&pr->driver);
> + mutex_unlock(&pi_mutex);
Sashiko had a comment about this that I think is very valid: if rmmod is
executed with devices attached, what happens here?
This entire driver seems to be lacking reference counting on the
modules/drivers, so this all seems very fragile.
> +
> }
> EXPORT_SYMBOL_GPL(pata_parport_unregister_driver);
>
> @@ -646,6 +648,7 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
> port_wanted = -1;
> }
>
> + guard(mutex)(&pi_mutex);
Please no. We do not use these annotations anywhere in libata and I personally
consider anything that hides code a *bad* idea. So just move the mutex_lock()
call here and add the missing mutex_unlock() in the !drv case below.
> drv = driver_find(protocol, &pata_parport_bus_type);
> if (!drv) {
> if (strcmp(protocol, "auto")) {
> @@ -656,15 +659,12 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
> } else {
> pr_wanted = container_of(drv, struct pi_protocol, driver);
> }
> -
> - mutex_lock(&pi_mutex);
> /* walk all parports */
> idr_for_each_entry(&parport_list, parport, port_num) {
> if (port_num == port_wanted || port_wanted == -1) {
> parport = parport_find_number(port_num);
> if (!parport) {
> pr_err("no such port %s\n", port);
> - mutex_unlock(&pi_mutex);
> return -ENODEV;
> }
> /* walk all protocols */
> @@ -677,7 +677,6 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
> parport_put_port(parport);
> }
> }
> - mutex_unlock(&pi_mutex);
> if (!ok)
> return -ENODEV;
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: pata_parport: Fix use-after-free in new_device_store
2026-07-29 23:42 ` Damien Le Moal
@ 2026-07-30 2:58 ` Pei Xiao
0 siblings, 0 replies; 6+ messages in thread
From: Pei Xiao @ 2026-07-30 2:58 UTC (permalink / raw)
To: Damien Le Moal, cassel, linux, axboe, linux-ide, linux-kernel
Cc: shuangpeng.kernel
在 2026/7/30 07:42, Damien Le Moal 写道:
> On 7/29/26 21:27, Pei Xiao wrote:
>> The function new_device_store() calls driver_find() without any
>> protection against concurrent driver unregistration. This can lead
>> to a use-after-free (UAF) when a driver is unloaded (via rmmod)
>> in parallel with a new device addition via sysfs.
>>
>> The race window exists because driver_find() returns a pointer to
>> the driver's private data, but does not increase its reference
>> count. The caller is responsible for ensuring the driver remains
>> valid, but new_device_store() did not hold any lock or reference
>> during the lookup and subsequent use.
>>
>> Concurrently, pata_parport_unregister_driver() releases the
>> pi_mutex before calling driver_unregister(), allowing new_device_store
>> to proceed with a stale pointer after the driver has been freed.
>>
>> Fix this by expanding the critical section protected by pi_mutex
>> in both functions:
>>
>> - In new_device_store(): acquire pi_mutex before calling driver_find(),
>> and keep it held until all uses of the found driver pointer are
>> completed.
>> - In pata_parport_unregister_driver(): hold pi_mutex throughout the
>> entire unregistration process, including the driver_unregister()
>> call, so that no concurrent lookup can see a partially removed
>> driver.
>>
>> Thread A (new_device_store) | Thread B (pata_parport_unregister_driver)
>> driver_find("aten") |
>> | mutex_lock(&pi_mutex)
>> | idr_remove(&protocols, id)
>> | mutex_unlock(&pi_mutex)
>> | driver_unregister(&pr->driver)
>> | (frees driver_private)
>> /* continues using stale driv */|
>> -> UAF! |
>>
>> Logs:
>> BUG: KASAN: slab-use-after-free in driver_find+0xd0/0xd4
>> Read of size 8 at addr ffffff9f970dc690 by task sh/4737
>> Call trace:
>> show_stack+0x14/0x1c (C)
>> dump_stack_lvl+0x70/0x84
>> print_report+0xf4/0x5a4
>> kasan_report+0xa0/0xe4
>> __asan_report_load8_noabort+0x18/0x20
>> driver_find+0xd0/0xd4
>> new_device_store+0x140/0x30c [pata_parport]
>> bus_attr_store+0x5c/0x94
>> sysfs_kf_write+0x1c4/0x25c
>> ...
>>
>> Allocated by task 4734:
>> kasan_save_stack+0x28/0x4c
>> kasan_save_track+0x1c/0x34
>> kasan_save_alloc_info+0x3c/0x4c
>> __kasan_kmalloc+0x98/0xac
>> __kmalloc_cache_noprof+0x158/0x3cc
>> bus_add_driver+0x70/0x4d8
>> driver_register+0xf0/0x3b0
>> pata_parport_register_driver+0xb8/0x1c4 [pata_parport]
>> 0xffffffc07f5ce014
>> do_one_initcall+0xb8/0x36c
>> do_init_module+0x230/0x6d4
>> ...
>>
>> Freed by task 4737:
>> kasan_save_stack+0x28/0x4c
>> kasan_save_track+0x1c/0x34
>> kasan_save_free_info+0x48/0x8c
>> __kasan_slab_free+0x5c/0x84
>> kfree+0x174/0x3e0
>> driver_release+0x1c/0x7c
>> kobject_put+0x178/0x47c
>> driver_find+0x78/0xd4
>> new_device_store+0x140/0x30c [pata_parport]
>> bus_attr_store+0x5c/0x94
>> sysfs_kf_write+0x1c4/0x25c
>> ...
>>
>> Fixes: 72f2b0b21850 ("drivers/block: Move PARIDE protocol modules to drivers/ata/pata_parport")
>> Reported-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
>> Closes: https://lore.kernel.org/lkml/20260728024015.2014674-1-shuangpeng.kernel@gmail.com/
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
>> ---
>> drivers/ata/pata_parport/pata_parport.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>> index 40baeac594a9..34f09192a419 100644
>> --- a/drivers/ata/pata_parport/pata_parport.c
>> +++ b/drivers/ata/pata_parport/pata_parport.c
>> @@ -3,6 +3,7 @@
>> * Copyright 2023 Ondrej Zary
>> * based on paride.c by Grant R. Guenther <grant@torque.net>
>> */
>> +#include <linux/cleanup.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/parport.h>
>> @@ -618,8 +619,9 @@ void pata_parport_unregister_driver(struct pi_protocol *pr)
>> break;
>> }
>> idr_remove(&protocols, id);
>> - mutex_unlock(&pi_mutex);
>> driver_unregister(&pr->driver);
>> + mutex_unlock(&pi_mutex);
>
> Sashiko had a comment about this that I think is very valid: if rmmod is
> executed with devices attached, what happens here?
> This entire driver seems to be lacking reference counting on the
> modules/drivers, so this all seems very fragile.
Ok, I will check this issue then.
>
>> +
>> }
>> EXPORT_SYMBOL_GPL(pata_parport_unregister_driver);
>>
>> @@ -646,6 +648,7 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
>> port_wanted = -1;
>> }
>>
>> + guard(mutex)(&pi_mutex);
>
> Please no. We do not use these annotations anywhere in libata and I personally
> consider anything that hides code a *bad* idea. So just move the mutex_lock()
> call here and add the missing mutex_unlock() in the !drv case below.
Ok.
Thanks!
Pei.
>
>> drv = driver_find(protocol, &pata_parport_bus_type);
>> if (!drv) {
>> if (strcmp(protocol, "auto")) {
>> @@ -656,15 +659,12 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
>> } else {
>> pr_wanted = container_of(drv, struct pi_protocol, driver);
>> }
>> -
>> - mutex_lock(&pi_mutex);
>> /* walk all parports */
>> idr_for_each_entry(&parport_list, parport, port_num) {
>> if (port_num == port_wanted || port_wanted == -1) {
>> parport = parport_find_number(port_num);
>> if (!parport) {
>> pr_err("no such port %s\n", port);
>> - mutex_unlock(&pi_mutex);
>> return -ENODEV;
>> }
>> /* walk all protocols */
>> @@ -677,7 +677,6 @@ static ssize_t new_device_store(const struct bus_type *bus, const char *buf, siz
>> parport_put_port(parport);
>> }
>> }
>> - mutex_unlock(&pi_mutex);
>> if (!ok)
>> return -ENODEV;
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-07-30 2:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-29 12:27 [PATCH] ata: pata_parport: Fix use-after-free in new_device_store Pei Xiao
2026-07-29 12:48 ` sashiko-bot
2026-07-29 13:10 ` Niklas Cassel
2026-07-30 1:47 ` Pei Xiao
2026-07-29 23:42 ` Damien Le Moal
2026-07-30 2:58 ` Pei Xiao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox