| Message ID | 20241113052050.1862420-1-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | dm: core: ofnode: safely fallback in ofnode_lookup_fdt | expand |
Hi Caleb, On Tue, 12 Nov 2024 at 22:21, Caleb Connolly <caleb.connolly@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@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. I think we should panic. Do you know what code path gets you here? More work is needed in livetree to sort of the rough edges, sadly... Regards, Simon
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@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@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. > 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 :/ Guess I'll drop this from my dev branch and come back when I figure this out again. > > More work is needed in livetree to sort of the rough edges, sadly... > > Regards, > Simon
Hi Caleb, On Wed, 27 Nov 2024 at 10:12, Caleb Connolly <caleb.connolly@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@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@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
On 27/11/2024 18:51, Simon Glass wrote: > Hi Caleb, > > On Wed, 27 Nov 2024 at 10:12, Caleb Connolly <caleb.connolly@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@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@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
Hi Caleb, On Thu, 28 Nov 2024 at 05:33, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 27/11/2024 18:51, Simon Glass wrote: > > Hi Caleb, > > > > On Wed, 27 Nov 2024 at 10:12, Caleb Connolly <caleb.connolly@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@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@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? Well, we don't have malloc() running so can't create the livetree that early. Could you pick up that info from the DT before relocation and store the info somewhere? It seems like that would be a pain, from my reading of carve_out_reserved_memory() So another idea would be to add a gd flag indicating that multi-tree is available. Before that we return the control FDT, similar to your patch. But it would be deterministic. > > > > 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
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 {
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@linaro.org> --- drivers/core/ofnode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)