Re: [PATCH 1/5] extcon: intel-int3496: drain ID work before its GPIOs and extcon device are freed

[Hans de Goede]

Hi,

On 6-Nov-26 14:17, hyunjungg wrote:
> From: Hyunjung Ko <hj351016@xxxxxxxxx>
> 
> int3496_probe() registers the debounce work for device-managed
> cancellation before it acquires any of the resources that the work
> handler dereferences:
> 
> 	devm_delayed_work_autocancel(dev, &data->work, int3496_do_usb_id);
> 	...
> 	data->gpio_usb_id  = devm_gpiod_get(dev, "id", ...);
> 	data->gpio_vbus_en = devm_gpiod_get(dev, "vbus", ...);
> 	data->gpio_usb_mux = devm_gpiod_get(dev, "mux", ...);
> 	data->edev = devm_extcon_dev_allocate(dev, int3496_cable);
> 	devm_extcon_dev_register(dev, data->edev);
> 	devm_request_threaded_irq(dev, data->usb_id_irq, ...);
> 
> devres releases resources in reverse registration order, so on unbind
> the extcon device and all three GPIO descriptors are released first and
> the work is cancelled last:
> 
> 	/* free_irq() */
> 	devm_extcon_dev_unregister() / extcon freed
> 	gpiod_put(mux) / gpiod_put(vbus_en) / gpiod_put(usb_id)
> 	...
> 	cancel_delayed_work_sync(&data->work);		/* too late */
> 
> int3496_thread_isr() arms the work with mod_delayed_work() on every ID
> pin edge, so a work instance can be pending or running at unbind. The
> handler int3496_do_usb_id() dereferences data->gpio_usb_id
> (gpiod_get_value_cansleep), data->gpio_usb_mux, data->gpio_vbus_en and
> data->edev (extcon_set_state_sync) with no guard. If it runs in the
> window after the GPIOs/extcon device have been released but before the
> work is cancelled, it operates on freed resources -> use-after-free.
> 
> Register the work for cancellation after the GPIOs and extcon device
> are acquired and registered (and before the IRQ, which arms it), so
> devres (LIFO) frees the IRQ first, then cancels the work, and only then
> releases the GPIOs and extcon device. devm_delayed_work_autocancel()
> still performs INIT_DELAYED_WORK() before the work is first queued
> (queue_delayed_work() below, and the IRQ handler, both run after it).
> 
> Found by static analysis / code review; compile-tested only, not
> reproduced on hardware (requires an INT3496 ACPI USB-ID mux device).
> 
> Fixes: f94a5becabf4 ("extconn: Clean-up few drivers by using managed work init")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hyunjung Ko <hj351016@xxxxxxxxx>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans



> ---
> diff --git a/drivers/extcon/extcon-intel-int3496.c b/drivers/extcon/extcon-intel-int3496.c
> index 7d16d5b..40774fb 100644
> --- a/drivers/extcon/extcon-intel-int3496.c
> +++ b/drivers/extcon/extcon-intel-int3496.c
> @@ -130,9 +130,6 @@ static int int3496_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	data->dev = dev;
> -	ret = devm_delayed_work_autocancel(dev, &data->work, int3496_do_usb_id);
> -	if (ret)
> -		return ret;
>  
>  	data->gpio_usb_id =
>  		devm_gpiod_get(dev, "id", GPIOD_IN | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
> @@ -169,6 +166,10 @@ static int int3496_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = devm_delayed_work_autocancel(dev, &data->work, int3496_do_usb_id);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_request_threaded_irq(dev, data->usb_id_irq,
>  					NULL, int3496_thread_isr,
>  					IRQF_SHARED | IRQF_ONESHOT |