Re: [PATCH 3/3] firmware: qcom: scm: drop the __scm global

[sashiko-bot@kernel.org]

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

New issues:
- [High] Module parameter updates can silently fail to reach the hardware during driver probe due to the behavior of driver_find_next_device().

Pre-existing issues:
- [High] Potential Use-After-Free in qcom_scm_get() and its callers due to unprotected devres pointers.
--

commit 03814e83233282a25e37723ed4eb68479514c4e0
Author: Albert Esteve <aesteve@redhat.com>

firmware: qcom: scm: drop the __scm global
    
Stop publishing a driver-wide singleton. Mark the instance
ready with scm->available, look it up from the platform driver
for module-parameter stores, and use platform_get_drvdata() on
shutdown.

> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 73e9752e3cca1..68cd8dc622c6f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c

[ ... ]

> @@ -2657,7 +2658,7 @@ static void qcom_scm_qtee_init(struct qcom_scm *scm)
>  bool qcom_scm_is_available(struct qcom_scm *scm)
>  {
>  	/* Paired with smp_store_release() in qcom_scm_probe */
> -	return scm && scm == smp_load_acquire(&__scm);
> +	return scm && smp_load_acquire(&scm->available);

[Severity: High]
This is a pre-existing issue, but does this introduce a new use-after-free
dereference site?

Looking at qcom_scm_get(), it acquires a device reference, retrieves the
devres-managed scm pointer, and immediately drops the reference:

struct qcom_scm *qcom_scm_get(void)
{
	...
	scm = dev_get_drvdata(dev);
	put_device(dev);

	if (!qcom_scm_is_available(scm))
		return NULL;
	...
}

If the device is concurrently unbound (e.g., via dynamic DT overlay removal),
device_unbind_cleanup() will synchronously free all devres allocations,
including the scm pointer.

Right after this in qcom_scm_get(), calling qcom_scm_is_available(scm)
will now dereference scm->available on the freed pointer. Can the pointer
be safeguarded before dereferencing it here?

>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_is_available);
>  
> @@ -2817,7 +2818,7 @@ static int set_download_mode(const char *val, const struct kernel_param *kp)
>  
>  	download_mode = ret;
>  	/* Pairs with smp_store_release() in qcom_scm_probe(). */
> -	scm = smp_load_acquire(&__scm);
> +	scm = qcom_scm_get();

[Severity: High]
Could this cause module parameter updates to silently fail during driver probe?

If a user or script updates the download_mode module parameter via sysfs while
qcom_scm_probe() is executing, set_download_mode() will call qcom_scm_get().

Because qcom_scm_get() uses driver_find_next_device(), which relies on
klist_devices, it will return NULL until driver_bound() adds the device
to the list after probe completes.

Since this skips the hardware update when qcom_scm_get() returns NULL, would
the global download_mode variable become permanently desynchronized from the
hardware state?

>  	if (scm)
>  		qcom_scm_set_download_mode(scm, download_mode);
>  
> @@ -2858,7 +2859,7 @@ static int set_minidump_dest(const char *val, const struct kernel_param *kp)
>  	minidump_dest = minidump_dest_map[i].val;
>  
>  	/* Pairs with smp_store_release() in qcom_scm_probe(). */
> -	scm = smp_load_acquire(&__scm);
> +	scm = qcom_scm_get();

[Severity: High]
Similarly to the download_mode parameter, could writing to minidump_dest via
sysfs during probe fail to reach the hardware?

Since qcom_scm_get() will return NULL before driver_bound() finishes, it
appears the minidump_sram write could be completely missed.

>  	if (scm && scm->minidump_sram && (download_mode & QCOM_DLOAD_MINIDUMP))
>  		writel_relaxed(minidump_dest, scm->minidump_sram);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-scm-device-api-v1-0-3573e2596c51@redhat.com?part=3