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

[Cristian Marussi <cristian.marussi@arm.com>]

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>

Thanks,
Cristian