Re: [PATCH] firmware: arm_scmi: Fix OOB in scmi_power_name_get()

[Dan Carpenter <error27@gmail.com>]

On Tue, May 19, 2026 at 09:36:40AM +0100, Cristian Marussi wrote:
> On Fri, May 15, 2026 at 01:10:56PM +0100, Cristian Marussi wrote:
> > On Fri, May 15, 2026 at 02:00:24PM +0200, Geert Uytterhoeven wrote:
> > > Hi Cristian,
> > > 
> > > On Fri, 15 May 2026 at 13:46, Cristian Marussi <cristian.marussi@arm.com> wrote:
> > > > On Fri, May 15, 2026 at 01:29:27PM +0200, Geert Uytterhoeven wrote:
> > > > > On Fri, 15 May 2026 at 12:28, Dan Carpenter <error27@gmail.com> wrote:
> > > > > > On Fri, May 15, 2026 at 11:59:15AM +0200, Geert Uytterhoeven wrote:
> > > > > > > scmi_power_name_get() does not validate the domain number passed by the
> > > > > > > external caller, which may lead to an out-of-bounds access.
> > > > > >
> > > > > > Is an external caller an out of tree caller?  So far as I can see this
> > > > >
> > > > > I meant a caller outside drivers/firmware/arm_scmi/.
> > > > >
> > > > > > is only called by scmi_pm_domain_probe().
> > > > > >
> > > > > >         scmi_pd->name = power_ops->name_get(ph, i);
> > > > > >
> > > > > > where i < num_domains.
> > > > >
> > > > > You are right. But this seems to be only API implementation in
> > > > > drivers/firmware/arm_scmi/ that does not validate the passed domain
> > > > > number.
> > > >
> > > > Yes we tend to validate protocol operations calls even if apparently
> > > > safe from teh caller perspective...indeed I have this fixed locally
> > > > since ages in an horrible patch, that does a lot more, and that I
> > > > never posted :P
> > > >
> > > > Usually, if it is worth, we also build an internal domain get helper to
> > > > reuse across the protocol unit...but here really there are only 2 call-sites.
> > > >
> > > > What I am not sure is what to return: "unknown" is safer as of now than NULL
> > > > for sure, but really, what happened is NOT that the name was "unknown" (which
> > > > by itself would be out-of-spec behaviour) it is more that the whole domain that
> > > > was referred to that was invalid and NOT existent...
> > > >
> > > > ....mmm I suppose we are opening another can of worms here :P
> > > 
> > > Like scmi_perf_info_get() returning ERR_PTR(-EINVAL) instead of NULL,
> > > and scmi_perf_domain_probe() never checking the return value anyway?
> > 
> > ...oh probably more than that...and related vendor FW that already exploits
> > these missing checks here and there to arbitrarily skip domains and return
> > out-of-spec non-contigous sets of domains becasue they cannot bother to
> > implement properly the spec (or they have simply forked their codebase from
> > an old drop and never updated it again...)...so that any kernel-side fix
> > you made along the road carries the risk of breaking something and a string
> > of possibly needed quirks...
> 
> Anyway, it is the safest option on the table until proper checks are in place.
> 
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

If it has a description like this then it's absolutely going to get a CVE
assigned.  We're used to hundreds of CVEs and all but I really feel like
this is a bad habit.

regards,
dan carpenter