Re: [PATCH v2] firmware: qcom: scm: Add check for NULL-pointer dereference

[Bjorn Andersson]

On Fri, Sep 20, 2024 at 11:43:17PM GMT, Wasim Nazir wrote:
> Avoid NULL pointer dereference while using any qcom SCM calls.
> Add macro to easy the check at each SCM calls.
> 

We already have a way to deal with this in the general case. Client
drivers should call qcom_scm_is_available() before calling the scm
interface.

Unfortunately your commit message makes it impossible to know if you're
referring to a case where this wasn't done, or isn't possible, or if
you've hit a bug.

> Changes in v2:
> - Cleanup in commit-message

This goes below the '---', by the diffstat. I don't know why you don't
have a diffstat, please figure out how to make your patches looks like
everyone else's.

> 
> Signed-off-by: Wasim Nazir <quic_wasimn@xxxxxxxxxxx>
> 
> diff --git a/drivers/firmware/qcom/qcom_scm-legacy.c b/drivers/firmware/qcom/qcom_scm-legacy.c
> index 029e6d117cb8..3247145a6583 100644
> --- a/drivers/firmware/qcom/qcom_scm-legacy.c
> +++ b/drivers/firmware/qcom/qcom_scm-legacy.c
> @@ -148,6 +148,9 @@ int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>  	__le32 *arg_buf;
>  	const __le32 *res_buf;
> 
> +	if (!dev)
> +		return -EPROBE_DEFER;

-EPROBE_DEFER only makes sense to the caller during probe. In all other
cases this is an invalid error value.

> +
>  	cmd = kzalloc(PAGE_ALIGN(alloc_len), GFP_KERNEL);
>  	if (!cmd)
>  		return -ENOMEM;
[..]
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
[..]
> @@ -387,7 +397,7 @@ static int qcom_scm_set_boot_addr(void *entry, const u8 *cpu_bits)
>  	desc.args[0] = flags;
>  	desc.args[1] = virt_to_phys(entry);
> 
> -	return qcom_scm_call_atomic(__scm ? __scm->dev : NULL, &desc, NULL);
> +	return qcom_scm_call_atomic(__scm->dev, &desc, NULL);

I don't think you understand why this is written the way it is.

>  }
> 
[..]
> @@ -1986,6 +2113,13 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	/* Let all above stores be available after this */
>  	smp_store_release(&__scm, scm);
> 
> +	__scm->reset.ops = &qcom_scm_pas_reset_ops;
> +	__scm->reset.nr_resets = 1;
> +	__scm->reset.of_node = pdev->dev.of_node;
> +	ret = devm_reset_controller_register(&pdev->dev, &__scm->reset);
> +	if (ret)
> +		return ret;
> +

Why did this move?

Regards,
Bjorn

>  	irq = platform_get_irq_optional(pdev, 0);
>  	if (irq < 0) {
>  		if (irq != -ENXIO)
> --
> 2.46.1
>