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

On Tue, May 19, 2026 at 11:46:55AM +0300, Dan Carpenter wrote:
> 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 <[email protected]> wrote:
> > > > > On Fri, May 15, 2026 at 01:29:27PM +0200, Geert Uytterhoeven wrote:
> > > > > > On Fri, 15 May 2026 at 12:28, Dan Carpenter <[email protected]> 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 <[email protected]>
> 
> 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.

Indeed, I think already happened to get a CVE on this internal improved
checks despite the fix was more a defensive thing since it had no chance
to be exposed by the current code.

I could be wrong but I think that this is one of the reason I started using
(and maybe abusing) for similar patches the "Harden protocol...." commmit-msg
because it really does not qualify as Fixes for backporting as specified in [1]

[1]: https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst

...so at the end probably worth to drop Fixes as you said earlier...

Thanks,
Cristian