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

[Wasim Nazir]

On Sat, Oct 05, 2024 at 09:46:26PM -0500, Bjorn Andersson wrote:
> 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.
My intention is to check all corner cases and provide proper error logs
wherever the check fails.

There is no active case/example where it is failing but irrespective of
client (using qcom_scm_is_available()) or driver using any SCM calls,
want to add this check so that we don't need to fall into case
where we need debugging of NULL check and error logs are enough
to detect the problem.
> 
> 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.

Will make this correction in next patch.
> 
> > 
> > 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.

I am returning EPROBE_DEFER so that any probe can use the return value
to retry while at non-probe place it can be treated as normal failure
(-ve value return).
Please let me know if anything better can be used at this place.
> 
> > +
> >  	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.
Here I am removing this check as before reaching here __scm variable is
already checked for validity.
> 
> >  }
> > 
> [..]
> > @@ -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?
&qcom_scm_pas_reset_ops is the first ops which might use __scm, so moving its
registration below smp_store_release(&__scm, scm) so that __scm is set
before utilizing in reset-ops.
> 
> Regards,
> Bjorn
> 
> >  	irq = platform_get_irq_optional(pdev, 0);
> >  	if (irq < 0) {
> >  		if (irq != -ENXIO)
> > --
> > 2.46.1
> >

Regards,
Wasim