Re: [PATCH 1/3] HID: amd_sfh: Add accessor to read the operating-mode sensor

[sashiko-bot@kernel.org]

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

New issues:
- [High] The auxiliary device created for the tablet-mode switch is never destroyed, leaking memory and leaving dangling device registrations.
- [High] Reading the operating mode on a v1.0 device accesses a register that is corrupted by DMA addresses, returning invalid data.
- [Medium] The `amd_sfh_deinit_emp2_action` devres action is registered prematurely and unconditionally clears the global `emp2` pointer, potentially corrupting global state.
- [Medium] The global `emp2` pointer is published before asynchronous initialization completes, causing data races on dynamically populated structures.

Pre-existing issues:
- [High] Exported APIs accessing the global `emp2` pointer lack synchronization, leading to a potential use-after-free and unmapped MMIO access if the device is concurrently unbound.
--

--- Patch [1]: [PATCH 1/3] HID: amd_sfh: Add accessor to read the operating-mode sensor ---
commit 1243ba22a0e430d7c20b9d71ce0f64ee46332ff2
Author: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

HID: amd_sfh: Add accessor to read the operating-mode sensor

Allow other drivers to query the operating mode (laptop or tablet)
reported by the Sensor Fusion Hub. This is the interface used by the
tablet-mode switch driver to report the device posture to userspace.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index 96ae792beeb6c..0f5346d2174c2 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
[ ... ]
> @@ -383,3 +383,19 @@ int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata)
>  
>  	return 0;
>  }
> +
> +bool amd_sfh_op_idx_enabled(struct amd_mp2_dev *mp2)
> +{
> +	struct amdtp_cl_data *cl = mp2->cl_data;
> +	int i;
> +
> +	if (!cl)
> +		return false;
> +
> +	for (i = 0; i < cl->num_hid_devices; i++)
> +		if (cl->sensor_idx[i] == op_idx &&
> +		    cl->sensor_sts[i] == SENSOR_ENABLED)
> +			return true;
> +
> +	return false;
> +}

[Severity: High]
Does enabling this operating mode index leak the auxiliary device?

When amd_sfh_op_idx_enabled() returns true, it appears to trigger the
creation of mp2->tm_auxdev in amd_sfh_maybe_register_tm():

drivers/hid/amd-sfh-hid/amd_sfh_pcie.c:amd_sfh_maybe_register_tm() {
	...
	present = mp2->sfh1_1_ops ? mp2->dev_en.is_sra_present
				  : amd_sfh_op_idx_enabled(mp2);
	if (!present)
		return;

	mp2->tm_auxdev = auxiliary_device_create(&mp2->pdev->dev, KBUILD_MODNAME,
						 "tabletmode", NULL, 0);
	...
}

However, amd_mp2_pci_remove() does not seem to destroy this auxiliary
device, which could leave a dangling sysfs registration and leak memory
if the device is unbound.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index 4b81cebdc3359..a3672a9302c5d 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
[ ... ]
> @@ -471,6 +477,10 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
>  	if (rc)
>  		return rc;
>  
> +	rc = devm_add_action_or_reset(&pdev->dev, amd_sfh_deinit_emp2_action, NULL);
> +	if (rc)
> +		return rc;
> +

[Severity: Medium]
Could this unconditionally clear the global emp2 pointer on a probe failure?

If devm_add_action_or_reset() is called before the device successfully
assigns its privdata to emp2 (which happens later in probe), and a
subsequent probe step fails, the devres cleanup will fire.

Will this wipe the emp2 pointer via sfh_deinit_emp2() even if it is
currently owned by another active device in a multi-device scenario?

>  	privdata->sfh1_1_ops = (const struct amd_sfh1_1_ops *)id->driver_data;
>  	if (privdata->sfh1_1_ops) {
>  		if (boot_cpu_data.x86 >= 0x1A)
> @@ -480,6 +490,7 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
>  		if (rc)
>  			return rc;
>  
> +		sfh_set_emp2(privdata);
>  		schedule_work(&privdata->work);
>  		return 0;
>  	}

[Severity: Medium]
Is there a data race when publishing the device globally before the workqueue
finishes?

sfh_set_emp2() exposes the device to exported APIs like amd_get_sfh_info().
If that API is called concurrently, amd_sfh_op_idx_enabled() might locklessly
iterate over cl->num_hid_devices and cl->sensor_idx[] while they are still
being populated by the workqueue in amd_sfh_hid_client_init().

> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> index 837d59e7a6610..74b3d3382a82c 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
[ ... ]
> @@ -81,12 +81,43 @@ void sfh_deinit_emp2(void)
>  	emp2 = NULL;
>  }
>  
> +void sfh_set_emp2(struct amd_mp2_dev *mp2)
> +{
> +	emp2 = mp2;
> +}
> +
> +static int amd_sfh_op_mode_info(u32 *op_mode)
> +{
> +	struct sfh_op_mode mode;
> +	bool present;
> +
> +	if (!op_mode)
> +		return -EINVAL;
> +	if (!emp2)
> +		return -ENODEV;

[Severity: High]
This is a pre-existing issue, but does this lockless check of emp2 create a
race condition with device unbind?

If a privileged user unbinds the SFH PCI driver concurrently, the teardown
path unmaps the MMIO, frees privdata, and the devres action clears emp2.
Could the querying thread dereference emp2 after it has been freed if the
unbind happens immediately after this !emp2 check?

> +
> +	present = emp2->sfh1_1_ops ? emp2->dev_en.is_sra_present
> +				   : amd_sfh_op_idx_enabled(emp2);
> +	if (!present)
> +		return -ENODEV;
> +
> +	mode.val = readl(emp2->mmio + amd_get_c2p_val(emp2, 3));

[Severity: High]
Will this read corrupted data on v1.0 hardware?

On v1.0 hardware, amd_start_sensor() uses a 64-bit writeq() to AMD_C2P_MSG2
to program the sensor's DMA address:

drivers/hid/amd-sfh-hid/amd_sfh_pcie.c:amd_start_sensor() {
	...
	writeq(info.dma_address, privdata->mmio + AMD_C2P_MSG2);
	...
}

This 64-bit write spans both AMD_C2P_MSG2 and AMD_C2P_MSG3. Since
amd_get_c2p_val(emp2, 3) accesses AMD_C2P_MSG3, will it return the upper
32 bits of a DMA address instead of the actual operating mode?

> +	dev_dbg(&emp2->pdev->dev,
> +		"op-mode: raw=0x%08x mode=%u lid=%u angle=%u ontable=%u devmode=%u\n",
> +		mode.val, mode.op_mode.mode, mode.op_mode.lidstatus,
> +		mode.op_mode.angle, mode.op_mode.ontablestate,
> +		mode.op_mode.devicemode);
> +	*op_mode = mode.op_mode.mode;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721174422.3109166-1-Basavaraj.Natikar@amd.com?part=1