| Message ID | 7215874eab207352d52473a72b59433b8470ade8.1788349317.git.xiaopei01@kylinos.cn |
|---|---|
| State | New |
| Headers | show |
| Series | ata: pata_parport: fix UAF on protocol module unload | expand |
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 <xiaopei01@kylinos.cn> 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. > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c > index 7462f9b1acc57..4ee5188070319 100644 > --- a/drivers/ata/pata_parport/pata_parport.c > +++ b/drivers/ata/pata_parport/pata_parport.c [ ... ] > @@ -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? > + > + 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.
diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c index 7462f9b1acc5..4ee518807031 100644 --- a/drivers/ata/pata_parport/pata_parport.c +++ b/drivers/ata/pata_parport/pata_parport.c @@ -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);
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(+)