[PATCH] dm: core: ofnode: safely fallback in ofnode_lookup_fdt

[Simon Glass]

Hi Caleb,

On Wed, 27 Nov 2024 at 10:12, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Simon,
>
> On 27/11/2024 17:52, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Tue, 12 Nov 2024 at 22:21, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >> Under some conditions it's possible to hit the null condition here like
> >> when running with OF_LIVE and using the ofnode API before
> >> initr_of_live() is called. There is very little null
> >> checking for this in the FDT framework, so returning null here can
> >> result in weird null pointer conditions.
> >>
> >> Instead let's return the control FDT in the fallback case, this is
> >> usually what the user was expecting.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>  drivers/core/ofnode.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> >> index 950895e72a99..7a7f25fc537c 100644
> >> --- a/drivers/core/ofnode.c
> >> +++ b/drivers/core/ofnode.c
> >> @@ -152,9 +152,9 @@ void *ofnode_lookup_fdt(ofnode node)
> >>                 uint i = OFTREE_TREE_ID(node.of_offset);
> >>
> >>                 if (i >= oftree_count) {
> >>                         log_debug("Invalid tree ID %x\n", i);
> >> -                       return NULL;
> >> +                       return (void *)gd->fdt_blob;
> >>                 }
> >>
> >>                 return oftree_list[i];
> >>         } else {
> >> --
> >> 2.47.0
> >>
> >
> > Eek I really don't like that, since it will silently return an
> > unexpected value.
> What's the unexpected value? The only caller for this is ofnode_to_fdt()
> and the return value for that is never checked. It seems clear to me
> that in practise NULL is the unexpected value here.

Firstly, it looks like ofnode_lookup_fdt() should be static

If a node offset cannot be found, that suggests that oftree_list[] is
empty, which suggests that oftree_reset() has not been called yet.

oftree_reset() is called after relocation. Before relocation, it just
assumes that the control FDT is being used.

I wonder if you are hitting this problem after relocation, in one of
the functions in init_sequence_r[] before initr_dm()?

In any case this does seem like a bug that we should fix.

>
> > I think we should panic. Do you know what code path
> > gets you here?
>
> Hmm, I don't exactly recall. Should really have made a note when I wrote
> this patch :/

Yes, I know that feeling :-(

>
> Guess I'll drop this from my dev branch and come back when I figure this
> out again.

Yes OK. A patch which panics would be fine for now, if you like.


> >
> > More work is needed in livetree to sort of the rough edges, sadly...

Regards,
Simon