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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4j7qx24xwxo4l4bfagjob2pb2taadqlc22gxeiwjttfwrtqua7@eheozhig65gm>
Date: Tue, 8 Oct 2024 14:43:54 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Wasim Nazir <quic_wasimn@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] firmware: qcom: scm: Add check for NULL-pointer
 dereference

On Tue, Oct 08, 2024 at 02:09:15PM GMT, Wasim Nazir wrote:
> 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@...cinc.com>
> > > 
> > > 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.

Just drop it. This adds no benefits. If SCM has probed, then __scm->dev
is set. If it was not probed, then the code already oopsed by
dereferencing NULL __scm call.

> > 
> > > +
> > >  	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.

No, it wasn't (or somebody removed too much of the context)

> > 
> > >  }
> > > 
> > [..]
> > > @@ -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.

So scm->reset should have been prepared before smp_store_release() and
only devm_reset_controller_register() should be moved after
smp_store_release().

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

-- 
With best wishes
Dmitry