[PATCH] pps: parport: release port claim before unregistering PPS

[PATCH] pps: parport: release port claim before unregistering PPS

[Yaohan Li]

parport_detach() unregisters the PPS source before releasing the
parallel-port claim. Disabling interrupts does not necessarily drain
an interrupt handler already in progress, and the handler can still
reference the PPS source after it has been released.

Release the parport claim first. parport_release() synchronizes with
interrupt handlers using the parport claim lock, so the PPS source
remains valid until the existing handler has completed. Unregister
the PPS source only after the claim has been released.

A KASAN test using the actual pps_parport detach and interrupt paths
reproduced the use-after-free before the change and did not report
it under the same injected interleaving after the change.

Fixes: a10203c691eac ("pps: add parallel port PPS client")
Assisted-by: ZCode:GLM-4
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
---
 drivers/pps/clients/pps_parport.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
index f5af18bc2971a..8031446db8ada 100644
--- a/drivers/pps/clients/pps_parport.c
+++ b/drivers/pps/clients/pps_parport.c
@@ -209,8 +209,16 @@ static void parport_detach(struct parport *port)
 	device = pardev->private;
 
 	port->ops->disable_irq(port);
-	pps_unregister_source(device->pps);
+	/*
+	 * Release the port claim before unregistering the PPS source:
+	 * parport_release() takes cad_lock for writing, which drains any
+	 * in-flight interrupt handler (parport_generic_irq() calls our
+	 * irq_func under cad_lock for reading, touching device->pps).
+	 * With the old order the handler could still run after
+	 * pps_unregister_source() had freed the pps_device.
+	 */
 	parport_release(pardev);
+	pps_unregister_source(device->pps);
 	parport_unregister_device(pardev);
 	ida_free(&pps_client_index, device->index);
 	kfree(device);
-- 
2.34.1

Re: [PATCH] pps: parport: release port claim before unregistering PPS

[Rodolfo Giometti]

> parport_detach() unregisters the PPS source before releasing the
> parallel-port claim. Disabling interrupts does not necessarily drain
> an interrupt handler already in progress, and the handler can still
> reference the PPS source after it has been released.
>
> Release the parport claim first. parport_release() synchronizes with
> interrupt handlers using the parport claim lock, so the PPS source
> remains valid until the existing handler has completed. Unregister
> the PPS source only after the claim has been released.
>
> A KASAN test using the actual pps_parport detach and interrupt paths
> reproduced the use-after-free before the change and did not report
> it under the same injected interleaving after the change.
>
> Fixes: a10203c691eac ("pps: add parallel port PPS client")
> Assisted-by: ZCode:GLM-4
> Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
> ---
>  drivers/pps/clients/pps_parport.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
> index f5af18bc2971a..8031446db8ada 100644
> --- a/drivers/pps/clients/pps_parport.c
> +++ b/drivers/pps/clients/pps_parport.c
> @@ -209,8 +209,16 @@ static void parport_detach(struct parport *port)
>  	device = pardev->private;
>
>  	port->ops->disable_irq(port);
> -	pps_unregister_source(device->pps);
> +	/*
> +	 * Release the port claim before unregistering the PPS source:
> +	 * parport_release() takes cad_lock for writing, which drains any
> +	 * in-flight interrupt handler (parport_generic_irq() calls our
> +	 * irq_func under cad_lock for reading, touching device->pps).
> +	 * With the old order the handler could still run after
> +	 * pps_unregister_source() had freed the pps_device.
> +	 */

Just a nit, only if you need to respin: IMHO the last two lines of
this comment could go, since they describe the old code rather than
this one, and the commit message already says it.

>  	parport_release(pardev);
> +	pps_unregister_source(device->pps);
>  	parport_unregister_device(pardev);
>  	ida_free(&pps_client_index, device->index);
>  	kfree(device);

Acked-by: Rodolfo Giometti <giometti@enneenne.com>

Thanks,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming