Re: [PATCH v2 4/5] HID: amd_sfh: Register tablet-mode auxiliary device

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The tablet-mode auxiliary device is unregistered in the devres cleanup phase, which occurs after the parent driver has manually shut down the hardware in its `.remove` callback, creating a Use-After-Teardown window.
--

--- Patch [4]: [PATCH v2 4/5] HID: amd_sfh: Register tablet-mode auxiliary device ---
commit cb1f4966427e04f4389572394e9a4fb57915128b
Author: Basavaraj Natikar <[email protected]>

HID: amd_sfh: Register tablet-mode auxiliary device
    
Register an auxiliary device when the operating-mode sensor is present,
so a dedicated input driver can bind to it and report the device posture.
This keeps the input handling out of the sensor transport driver.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index 4b1cd260410d1..f6f016c8c6787 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
[ ... ]
> @@ -389,6 +391,50 @@ static const struct attribute_group *amd_sfh_groups[] = {
[ ... ]
> +static void amd_sfh_maybe_register_tm(struct amd_mp2_dev *mp2)
> +{
[ ... ]
> +	mp2->tm_auxdev = adev;
> +	if (devm_add_action_or_reset(&mp2->pdev->dev, amd_sfh_tm_cleanup, mp2))
> +		dev_warn(&mp2->pdev->dev, "tabletmode cleanup registration failed\n");
> +}
[ ... ]
> @@ -424,6 +471,7 @@ static void sfh_init_work(struct work_struct *work)
[ ... ]
>  static void amd_sfh_remove(struct pci_dev *pdev)
>  {
>  	struct amd_mp2_dev *mp2 = pci_get_drvdata(pdev);
>  
>  	flush_work(&mp2->work);
>  	if (mp2->init_done)
>  		mp2->mp2_ops->remove(mp2);

[Severity: High]
Does this create a use-after-teardown window on device unbind?

Because the tablet-mode auxiliary device cleanup is registered via
devm_add_action_or_reset() in amd_sfh_maybe_register_tm(), the auxiliary
device is only destroyed during the devres release phase.

Since devres cleanup runs after amd_sfh_remove() completes,
mp2->mp2_ops->remove(mp2) manually tears down the underlying hardware before
the auxiliary device is destroyed.

If the auxiliary driver's remove callback or userspace attempts to
interact with the device during this window between amd_sfh_remove() and the
devres cleanup phase, could they end up accessing disabled hardware?

>  }

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