[PATCH v2 0/2] ata: pata_parport: fix UAF on protocol module unload

[PATCH v2 0/2] ata: pata_parport: fix UAF on protocol module unload

[Pei Xiao]

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 | 28 ++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

-- 
2.25.1

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

[Pei Xiao]

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 <xiaopei01@kylinos.cn>
---
changes in v2:
1.remove out_unreg_dev: label
---
 drivers/ata/pata_parport/pata_parport.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index cf81a6128f55..046ab7e3adbc 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -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;
 
@@ -568,7 +573,6 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 		pi->proto->release_proto(pi);
 out_module_put:
 	module_put(pi->proto->owner);
-out_unreg_dev:
 	device_unregister(&pi->dev);
 	/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 	return NULL;
-- 
2.25.1

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

[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 <xiaopei01@kylinos.cn>
---
 drivers/ata/pata_parport/pata_parport.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 046ab7e3adbc..712209882ab7 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -611,6 +611,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;
@@ -622,6 +634,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);
 
-- 
2.25.1

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

[sashiko-bot]

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

Pre-existing issues:
- [High] pata_parport_attach fails for all parports except port 0
--

commit b043c5b13721ce815e34642853318549a000ffd3
Author: Pei Xiao <xiaopei01@kylinos.cn>

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
dynamically created devices untouched. Later removal of these dangling
devices dereferences freed module memory, crashing the kernel. This commit
tears down all pi_adapters using the protocol before unregistering the
driver.

> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 046ab7e3adbcf..712209882ab75 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
[ ... ]

While reviewing this, I noticed a pre-existing issue in pata_parport_attach():

drivers/ata/pata_parport/pata_parport.c:pata_parport_attach() {
    ...
	mutex_lock(&pi_mutex);
	id = idr_alloc(&parport_list, port, port->number, port->number,
		       GFP_KERNEL);
	if (id < 0) {
    ...
}

[Severity: High]
This is a pre-existing issue, but does this idr_alloc() call fail for all
parports except port 0?

Since the end parameter of idr_alloc() is exclusive, when port->number > 0,
it computes max = end - 1. Because start is equal to port->number, the
condition start > max evaluates to true, causing it to return -ENOSPC.

Would this cause attaching any parport with a number greater than 0 to
silently fail and completely break functionality for those ports?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788867690.git.xiaopei01@kylinos.cn?part=2

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

[Niklas Cassel]

On Tue, Sep 08, 2026 at 12:08:55PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] pata_parport_attach fails for all parports except port 0
> --
> 
> commit b043c5b13721ce815e34642853318549a000ffd3
> Author: Pei Xiao <xiaopei01@kylinos.cn>
> 
> 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
> dynamically created devices untouched. Later removal of these dangling
> devices dereferences freed module memory, crashing the kernel. This commit
> tears down all pi_adapters using the protocol before unregistering the
> driver.
> 
> > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> > index 046ab7e3adbcf..712209882ab75 100644
> > --- a/drivers/ata/pata_parport/pata_parport.c
> > +++ b/drivers/ata/pata_parport/pata_parport.c
> [ ... ]
> 
> While reviewing this, I noticed a pre-existing issue in pata_parport_attach():
> 
> drivers/ata/pata_parport/pata_parport.c:pata_parport_attach() {
>     ...
> 	mutex_lock(&pi_mutex);
> 	id = idr_alloc(&parport_list, port, port->number, port->number,
> 		       GFP_KERNEL);
> 	if (id < 0) {
>     ...
> }

> 
> [Severity: High]
> This is a pre-existing issue, but does this idr_alloc() call fail for all
> parports except port 0?
> 
> Since the end parameter of idr_alloc() is exclusive, when port->number > 0,
> it computes max = end - 1. Because start is equal to port->number, the
> condition start > max evaluates to true, causing it to return -ENOSPC.
> 
> Would this cause attaching any parport with a number greater than 0 to
> silently fail and completely break functionality for those ports?

Looking at idr_alloc(), this looks like a legit pre-existing issue.

There is another call to idr_alloc() i pata_parport_register_driver(),
but it does:
error = idr_alloc(&protocols, pr, 0, 0, GFP_KERNEL);

0,0 is start,end

if @end is <= 0, it is treated as one larger than %INT_MAX

So this is only a problem in pata_parport_attach().



Kind regards,
Niklas

Re: [PATCH v2 0/2] ata: pata_parport: fix UAF on protocol module unload

[Niklas Cassel]

On Tue, 08 Sep 2026 19:47:26 +0800, Pei Xiao wrote:
> 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.
> 
> [...]

Applied to libata/linux.git (for-7.4), thanks!

[1/2] ata: pata_parport: pin the protocol module before device_register()
      https://git.kernel.org/libata/linux/c/5d3355c7
[2/2] ata: pata_parport: unregister devices on protocol unregister
      https://git.kernel.org/libata/linux/c/d82c5cf6

Kind regards,
Niklas

Re: [PATCH v2 0/2] ata: pata_parport: fix UAF on protocol module unload

[Niklas Cassel]

On Wed, Sep 09, 2026 at 12:52:47PM +0200, Niklas Cassel wrote:
> On Tue, 08 Sep 2026 19:47:26 +0800, Pei Xiao wrote:
> > 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.
> > 
> > [...]
> 
> Applied to libata/linux.git (for-7.4), thanks!
> 
> [1/2] ata: pata_parport: pin the protocol module before device_register()
>       https://git.kernel.org/libata/linux/c/5d3355c7
> [2/2] ata: pata_parport: unregister devices on protocol unregister
>       https://git.kernel.org/libata/linux/c/d82c5cf6

This series was dropped because of kernel test robot reporting
build error.

(Interestingly, Sashiko did only reported an unrelated pre-existing issue,
but did not report the build error.)


Kind regards,
Niklas