ata: pata_parport: fix UAF on protocol module unload

PATCH Superseded Pei Xiao 3 participants 7 messages Sep 2 – Sep 8 libata

Share & tools

Pei Xiao

author

2026-09-02 11:51 UTC #0 lore raw

cover ata: pata_parport: fix UAF on protocol module unload

This series fixes use-after-free issues in pata_parport when a protocol module goes away while pi_adapter devices created by it are still attached.

Patch 1 pins the protocol module before the device becomes visible. Previously try_module_get() ran after device_register(), so a forced module unload in between left pi->proto dangling from the moment the device appeared on the bus.

Patch 2 makes pata_parport_unregister_driver() tear down all adapters using the protocol. Without this, the rollback path of a multi-protocol module init (e.g. kbic registering k951 then k971) left the devices of the already-registered protocol alive while the module loader freed the module memory; removing such a dangling device later crashed in pi_disconnect() dereferencing pi->proto->disconnect.

Pei Xiao (2):
ata: pata_parport: pin the protocol module before device_register()
ata: pata_parport: unregister devices on protocol unregister

 drivers/ata/pata_parport/pata_parport.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Signature · 1 line

2.25.1

Pei Xiao

author

2026-09-02 11:51 UTC #1 lore raw

PATCH 1/2 ata: pata_parport: pin the protocol module before device_register()

pi_init_one() calls device_register() before try_module_get(). Between these two calls the device is already visible but the module is not pinned yet, so an unload in this window leaves pi->proto dangling:

pi_init_one() rmmod -f <proto>
--------------------------------------------------------
device_register(&pi->dev)
device visible on the bus
module memory freed
pi->proto = pr <- writes into freed memory / dangles
try_module_get(...) <- too late, module already gone

Take the module reference before registering the device, and drop it on the device_register() failure path.

Fixes 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")

Signed-off-by Pei Xiao <[email protected]>

---
 drivers/ata/pata_parport/pata_parport.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

drivers/ata/pata_parport/pata_parport.c +9 -4

index cf81a6128f55..7462f9b1acc5 100644
@@ -509,6 +509,14 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 		return NULL;
 	}
 
+	pi->proto = pr;
+
+	if (!try_module_get(pi->proto->owner)) {
+		kfree(pi);
+		ida_free(&pata_parport_bus_dev_ids, id);
+		return NULL;
+	}
+
 	/* set up pi->dev before pi_probe_unit() so it can use dev_printk() */
 	pi->dev.parent = pata_parport_bus;
 	pi->dev.bus = &pata_parport_bus_type;

@@ -517,15 +525,12 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	pi->dev.id = id;
 	dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id);
 	if (device_register(&pi->dev)) {
+		module_put(pi->proto->owner);
 		put_device(&pi->dev);
 		/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 		return NULL;
 	}
 
-	pi->proto = pr;
-
-	if (!try_module_get(pi->proto->owner))
-		goto out_unreg_dev;
 	if (pi->proto->init_proto && pi->proto->init_proto(pi) < 0)
 		goto out_module_put;

Signature · 1 line

2.25.1

[email protected]

AI review

2026-09-02 12:06 UTC #2 lore raw

Re: [PATCH 1/2] ata: pata_parport: pin the protocol module before device_register()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Unused label `out_unreg_dev` causes compiler warning. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unused label out_unreg_dev causes compiler warning.
--

commit 99f7607699410f6446da6e37e3bed84251a51bd8
Author: Pei Xiao <[email protected]>

ata: pata_parport: pin the protocol module before device_register()

Take the module reference before registering the device to prevent a race condition where the device is visible but the protocol module is not yet pinned. Also ensure the reference is dropped on the device_register() failure path.

Quoted 4 lines

drivers/ata/pata_parport/pata_parport.c +0 -0
index cf81a6128f552..7462f9b1acc57 100644

[ ... ]

Quoted 4 lines

drivers/ata/pata_parport/pata_parport.c +0 -0

@@ -517,15 +525,12 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	pi->dev.id = id;
 	dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id);
 	if (device_register(&pi->dev)) {

drivers/ata/pata_parport/pata_parport.c static struct pi_adapter *pi_init_one(struct parport *parport, · line 528

Quoted 10 lines

drivers/ata/pata_parport/pata_parport.c +1 -4

+		module_put(pi->proto->owner);
 		put_device(&pi->dev);
 		/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 		return NULL;
 	}
 
-	pi->proto = pr;
-
-	if (!try_module_get(pi->proto->owner))
-		goto out_unreg_dev;

[Severity: Low]
Does removing this goto leave the out_unreg_dev label unused at the bottom of pi_init_one()?

If the label is no longer referenced, compiling the kernel will emit a -Wunused-label warning. This can cause build failures in configurations that enforce -Werror.

Quoted 2 lines

drivers/ata/pata_parport/pata_parport.c +0 -0

 	if (pi->proto->init_proto && pi->proto->init_proto(pi) < 0)
 		goto out_module_put;

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Niklas Cassel

maintainer

2026-09-04 16:29 UTC #3 lore raw

Re: [PATCH 1/2] ata: pata_parport: pin the protocol module before device_register()

Quoted 40 lines · On Wed, Sep 02, 2026 at 12:06:08PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unused label out_unreg_dev causes compiler warning.
--
commit 99f7607699410f6446da6e37e3bed84251a51bd8
Author: Pei Xiao <[email protected]>
ata: pata_parport: pin the protocol module before device_register()
Take the module reference before registering the device to prevent a race condition where the device is visible but the protocol module is not yet pinned. Also ensure the reference is dropped on the device_register() failure path.
Quoted 4 lines
drivers/ata/pata_parport/pata_parport.c +0 -0
index cf81a6128f552..7462f9b1acc57 100644
[ ... ]
Quoted 4 lines
drivers/ata/pata_parport/pata_parport.c +0 -0
@@ -517,15 +525,12 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	pi->dev.id = id;
 	dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id);
 	if (device_register(&pi->dev)) {
drivers/ata/pata_parport/pata_parport.c static struct pi_adapter *pi_init_one(struct parport *parport, · line 528
Quoted 10 lines
drivers/ata/pata_parport/pata_parport.c +1 -4
+		module_put(pi->proto->owner);
 		put_device(&pi->dev);
 		/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 		return NULL;
 	}
 
-	pi->proto = pr;
-
-	if (!try_module_get(pi->proto->owner))
-		goto out_unreg_dev;
[Severity: Low]
Does removing this goto leave the out_unreg_dev label unused at the bottom of pi_init_one()?

Quoted 3 lines

If the label is no longer referenced, compiling the kernel will emit a -Wunused-label warning. This can cause build failures in configurations that enforce -Werror.

We don't want to introduce any new build warnings.

W=1 builds are important too, even if not default.

Kind regards,
Niklas

Pei Xiao

author

2026-09-08 03:09 UTC #4 lore raw

Re: [PATCH 1/2] ata: pata_parport: pin the protocol module before device_register()

Quoted 41 lines · Niklas Cassel

Quoted 40 lines · On Wed, Sep 02, 2026 at 12:06:08PM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unused label out_unreg_dev causes compiler warning.
--
commit 99f7607699410f6446da6e37e3bed84251a51bd8
Author: Pei Xiao <[email protected]>
ata: pata_parport: pin the protocol module before device_register()
Take the module reference before registering the device to prevent a race condition where the device is visible but the protocol module is not yet pinned. Also ensure the reference is dropped on the device_register() failure path.
Quoted 4 lines
drivers/ata/pata_parport/pata_parport.c +0 -0
index cf81a6128f552..7462f9b1acc57 100644
[ ... ]
Quoted 4 lines
drivers/ata/pata_parport/pata_parport.c +0 -0
@@ -517,15 +525,12 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	pi->dev.id = id;
 	dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id);
 	if (device_register(&pi->dev)) {
drivers/ata/pata_parport/pata_parport.c static struct pi_adapter *pi_init_one(struct parport *parport, · line 528
Quoted 10 lines
drivers/ata/pata_parport/pata_parport.c +1 -4
+		module_put(pi->proto->owner);
 		put_device(&pi->dev);
 		/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 		return NULL;
 	}
 
-	pi->proto = pr;
-
-	if (!try_module_get(pi->proto->owner))
-		goto out_unreg_dev;
[Severity: Low]
Does removing this goto leave the out_unreg_dev label unused at the bottom of pi_init_one()?

Quoted 7 lines

Quoted 3 lines
If the label is no longer referenced, compiling the kernel will emit a -Wunused-label warning. This can cause build failures in configurations that enforce -Werror.
We don't want to introduce any new build warnings.
W=1 builds are important too, even if not default.

My fault. Thanks for pointing out. I will send v2

Pei.
Thanks!

Quoted 4 lines

Kind regards,
Niklas

Pei Xiao

author

2026-09-02 11:51 UTC #5 lore raw

PATCH 2/2 ata: pata_parport: unregister devices on protocol unregister

In reply to Pei Xiao

When a protocol module registers multiple protocols and a later registration fails (e.g. kbic_init registering k951 then k971), the rollback path calls pata_parport_unregister_driver() on the already registered protocol. This removes the protocol from the IDR and unregisters the driver, but leaves the dynamically created pi_adapter devices untouched.

Since the module init then fails, the module loader frees the module memory, bypassing the references held by the devices. Any later removal of these dangling devices (e.g. via sysfs delete_device or parport detach) hits pi_remove_one() -> pi_disconnect() -> pi->proto->disconnect, dereferencing the freed module memory and crashing the kernel.

Tear down all pi_adapters using the protocol in pata_parport_unregister_driver(), before driver_unregister(), while the module is still alive so the ->disconnect / ->release_proto callbacks are safe to call.

Fixes 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")

Signed-off-by Pei Xiao <[email protected]>

---
 drivers/ata/pata_parport/pata_parport.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

drivers/ata/pata_parport/pata_parport.c +14 -0

index 7462f9b1acc5..4ee518807031 100644
@@ -612,6 +612,18 @@ int pata_parport_register_driver(struct pi_protocol *pr)
 }
 EXPORT_SYMBOL_GPL(pata_parport_register_driver);
 
+static int pi_remove_by_proto(struct device *dev, void *data)
+{
+	struct pi_protocol *pr = data;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct pi_adapter *pi = host->private_data;
+
+	if (pi->proto == pr)
+		pi_remove_one(dev);
+
+	return 0;
+}
+
 void pata_parport_unregister_driver(struct pi_protocol *pr)
 {
 	struct pi_protocol *pr_iter;

@@ -623,6 +635,8 @@ void pata_parport_unregister_driver(struct pi_protocol *pr)
 			break;
 	}
 	idr_remove(&protocols, id);
+	/* remove adapters using this protocol while the module is still alive */
+	bus_for_each_dev(&pata_parport_bus_type, NULL, pr, pi_remove_by_proto);
 	driver_unregister(&pr->driver);
 	mutex_unlock(&pi_mutex);

Signature · 1 line

2.25.1

[email protected]

AI review

2026-09-02 12:07 UTC #6 lore raw

Re: [PATCH 2/2] ata: pata_parport: unregister devices on protocol unregister

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] AB-BA deadlock between pi_mutex and parport claim during device probing.
- [High] Missing NULL check on dev_get_drvdata() exposes a NULL pointer dereference if the driver is unbound via sysfs. --

commit 5b5d1ccae8e55ae326690dca369a3675ed2bc9bf
Author: Pei Xiao <[email protected]>
Subject: ata: pata_parport: unregister devices on protocol unregister

When a protocol module registers multiple protocols and a later registration fails, the rollback path unregisters the protocol but leaves the dynamically created pi_adapter devices untouched. This tear down ensures adapters are removed while the module is still alive.

Quoted 4 lines

drivers/ata/pata_parport/pata_parport.c +0 -0
index 7462f9b1acc57..4ee5188070319 100644

[ ... ]

drivers/ata/pata_parport/pata_parport.c int pata_parport_register_driver(struct pi_protocol *pr) · line 619

Quoted 9 lines

drivers/ata/pata_parport/pata_parport.c +5 -0

@@ -612,6 +612,18 @@ int pata_parport_register_driver(struct pi_protocol *pr)
 }
 EXPORT_SYMBOL_GPL(pata_parport_register_driver);
 
+static int pi_remove_by_proto(struct device *dev, void *data)
+{
+	struct pi_protocol *pr = data;
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct pi_adapter *pi = host->private_data;

[Severity: High]
This isn't a regression introduced by this patch, but does this missing null check expose a null pointer dereference if the driver is unbound via sysfs?

If a user unbinds the driver via sysfs, dev_set_drvdata(dev, NULL) is called. Later, when pi_remove_by_proto() or pi_remove_one() is triggered, wouldn't dev_get_drvdata() return NULL and cause a crash when dereferencing host->private_data?

drivers/ata/pata_parport/pata_parport.c int pata_parport_register_driver(struct pi_protocol *pr) · line 626

Quoted 7 lines

drivers/ata/pata_parport/pata_parport.c +7 -0
+
+	if (pi->proto == pr)
+		pi_remove_one(dev);
+
+	return 0;
+}
+

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there an AB-BA deadlock between pi_mutex and the parport claim during device probing?

During module initialization, we loop over all available protocols for a given parport while holding pi_mutex:

drivers/ata/pata_parport/pata_parport.c:pata_parport_attach() {
    ...
	mutex_lock(&pi_mutex);
    ...
	if (probe) {
		/* probe this port using all protocols */
		idr_for_each_entry(&protocols, pr, pr_num)
			pi_init_one(port, pr, -1, -1, -1);
	}
	mutex_unlock(&pi_mutex);
}

If a device successfully initializes with the first protocol, it permanently claims the parport:

drivers/ata/pata_parport/pata_parport.c:pi_connect() {
    ...
	parport_claim_or_block(pi->pardev);
    ...
}

The loop then proceeds to the next protocol and attempts to initialize it for the same port. Does the second initialization block forever while holding pi_mutex?

drivers/ata/pata_parport/pata_parport.c:pi_probe_unit() {
    ...
	if (pi->proto->test_port) {
		parport_claim_or_block(pi->pardev);
		max = pi->proto->test_port(pi);
		parport_release(pi->pardev);
	}
    ...
}

Because the port is permanently held by the first adapter, it appears the thread will block forever while holding pi_mutex, causing a system deadlock blocking all future parport ata operations.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2