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

[Caleb Connolly]

On 27/11/2024 18:51, Simon Glass wrote:
> 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()?

aha! Now I remember, mach-snapdragon has it's own enable_caches() which
looks at the DT to determine if we should do some additional work to
carve out reserved memory regions which some platforms might otherwise
speculate on triggering a crash.

I think this is also specific to some non-upstream patches I have which
use the ofnode API, since the current compatible check uses fdt_blob
directly.

Disabling OFNODE_MULTI_TREE works around the crash, which would be fine
since we don't currently use it on Qualcomm (I think?).

but it would be nice to solve this.

What about if we called oftree_reset() immediately after initr_reloc?

Kind regards,
> 
> 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

-- 
// Caleb (they/them)