[PATCH] fdt_get_phandle: Return invalid phandles as 0

[PATCH] fdt_get_phandle: Return invalid phandles as 0

[Pierre-Clément Tosi]

If a tree contains an invalid <u32> value as its <phandle> property,
mask it with '0' (invalid value) instead of returning it from
fdt_get_phandle().

Signed-off-by: Pierre-Clément Tosi <ptosi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
 libfdt/fdt_ro.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 9f6c551..7d1da6d 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -507,6 +507,7 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
 
 uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
 {
+	uint32_t phandle;
 	const fdt32_t *php;
 	int len;
 
@@ -519,7 +520,11 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
 			return 0;
 	}
 
-	return fdt32_ld_(php);
+	phandle = fdt32_ld_(php);
+	if (phandle > (uint32_t)FDT_MAX_PHANDLE)
+		phandle = 0;
+
+	return phandle;
 }
 
 const char *fdt_get_alias_namelen(const void *fdt,
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH] fdt_get_phandle: Return invalid phandles as 0

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

On Mon, Aug 01, 2022 at 01:31:34PM +0100, Pierre-Clément Tosi wrote:
> If a tree contains an invalid <u32> value as its <phandle> property,
> mask it with '0' (invalid value) instead of returning it from
> fdt_get_phandle().
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Hm.. I don't really see the point of this.  Because FDT_MAX_PHANDLE is
0xfffffffe (-2), the only effect this has is to change 0xffffffff (-1)
into 0 when returned.  Both are, indeed, invalid phandles, but they're
sometimes used to mean slightly different things in incomplete trees,
so it seems more useful to return these separately

> ---
>  libfdt/fdt_ro.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 9f6c551..7d1da6d 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -507,6 +507,7 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
>  
>  uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
>  {
> +	uint32_t phandle;
>  	const fdt32_t *php;
>  	int len;
>  
> @@ -519,7 +520,11 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
>  			return 0;
>  	}
>  
> -	return fdt32_ld_(php);
> +	phandle = fdt32_ld_(php);
> +	if (phandle > (uint32_t)FDT_MAX_PHANDLE)
> +		phandle = 0;
> +
> +	return phandle;
>  }
>  
>  const char *fdt_get_alias_namelen(const void *fdt,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] fdt_get_phandle: Return invalid phandles as 0

[Pierre-Clément Tosi]

On Tue, Aug 02, 2022 at 04:47:19PM +1000, David Gibson wrote:
> On Mon, Aug 01, 2022 at 01:31:34PM +0100, Pierre-Clément Tosi wrote:
> > If a tree contains an invalid <u32> value as its <phandle> property,
> > mask it with '0' (invalid value) instead of returning it from
> > fdt_get_phandle().
> > 
> > Signed-off-by: Pierre-Clément Tosi <ptosi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> Hm.. I don't really see the point of this.  Because FDT_MAX_PHANDLE is
> 0xfffffffe (-2), the only effect this has is to change 0xffffffff (-1)
> into 0 when returned.  Both are, indeed, invalid phandles, but they're
> sometimes used to mean slightly different things in incomplete trees,
> so it seems more useful to return these separately

I can see how returning the u32 as-is (without masking invalid values) can be
convenient in internal code giving a different meaning to the field as, in that
particular context, those "invalid" values actually become valid.

However, my worry is that, as this function is exposed by the library, client
code could rely on it and assume that any non-zero value it returns is valid:

    phandle = fdt_get_phandle(fdt, node);
    if (!phandle)
        /* handle error */
    /* (~0U) is considered valid! */

instead of the more verbose and less obvious

    if (!phandle || phandle == ~0U)
        /* handle error */

or (written to be robust against an evolving FDT_MAX_PHANDLE)

    if (!phandle || phandle > (uint32_t)FDT_MAX_PHANDLE)
        /* handle error */

leading to potential bugs.

To address your concern, if that extra meaning for phandles in incomplete trees
is only internal (not exposed out of libfdt), would it make sense to replace all
calls to fdt_get_phandle() by an internal function that behaves like
fdt_get_phandle() does today (returns the _actual_ phandle) and make
fdt_get_phandle() only return valid phandles?

Alternatively, libfdt could provide to its clients a function similar to
phandle_is_valid() (from dtc.h) and document in fdt_get_phandle() the need to
validate its result through that function? That wouldn't fix existing (buggy)
code but would at least make the API clearer.

Lastly, if none of the above can be implemented, the documentation of
fdt_get_phandle() should at least reflect the fact that its result, even if
non-zero, may not be a valid phandle.

> 
> > ---
> >  libfdt/fdt_ro.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index 9f6c551..7d1da6d 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -507,6 +507,7 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
> >  
> >  uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
> >  {
> > +	uint32_t phandle;
> >  	const fdt32_t *php;
> >  	int len;
> >  
> > @@ -519,7 +520,11 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
> >  			return 0;
> >  	}
> >  
> > -	return fdt32_ld_(php);
> > +	phandle = fdt32_ld_(php);
> > +	if (phandle > (uint32_t)FDT_MAX_PHANDLE)
> > +		phandle = 0;
> > +
> > +	return phandle;
> >  }
> >  
> >  const char *fdt_get_alias_namelen(const void *fdt,
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Pierre

Re: [PATCH] fdt_get_phandle: Return invalid phandles as 0

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4494 bytes --]

On Tue, Aug 02, 2022 at 02:44:37PM +0100, Pierre-Clément Tosi wrote:
> On Tue, Aug 02, 2022 at 04:47:19PM +1000, David Gibson wrote:
> > On Mon, Aug 01, 2022 at 01:31:34PM +0100, Pierre-Clément Tosi wrote:
> > > If a tree contains an invalid <u32> value as its <phandle> property,
> > > mask it with '0' (invalid value) instead of returning it from
> > > fdt_get_phandle().
> > > 
> > > Signed-off-by: Pierre-Clément Tosi <ptosi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > 
> > Hm.. I don't really see the point of this.  Because FDT_MAX_PHANDLE is
> > 0xfffffffe (-2), the only effect this has is to change 0xffffffff (-1)
> > into 0 when returned.  Both are, indeed, invalid phandles, but they're
> > sometimes used to mean slightly different things in incomplete trees,
> > so it seems more useful to return these separately
> 
> I can see how returning the u32 as-is (without masking invalid values) can be
> convenient in internal code giving a different meaning to the field as, in that
> particular context, those "invalid" values actually become valid.
> 
> However, my worry is that, as this function is exposed by the library, client
> code could rely on it and assume that any non-zero value it returns is valid:
> 
>     phandle = fdt_get_phandle(fdt, node);
>     if (!phandle)
>         /* handle error */
>     /* (~0U) is considered valid! */
> 
> instead of the more verbose and less obvious
> 
>     if (!phandle || phandle == ~0U)
>         /* handle error */

I see your point, and if we were designing the interface anew, that's
probably a better way to handle the error cases.  However, I think the
risk of errors like you describe is outweighted by the risks of
changing existing established behaviour for this function.

> or (written to be robust against an evolving FDT_MAX_PHANDLE)
> 
>     if (!phandle || phandle > (uint32_t)FDT_MAX_PHANDLE)
>         /* handle error */

FDT_MAX_PHANDLE really can't ever change.  Even back to the old OF
days, 0 and -1 were the only values that weren't valid phandles.
Since there may be existing trees out there using 0xfffffffe as a
valid phandle, we can't reduce FDT_MAX_PHANDLE.

> leading to potential bugs.
> 
> To address your concern, if that extra meaning for phandles in incomplete trees
> is only internal (not exposed out of libfdt), would it make sense to replace all
> calls to fdt_get_phandle() by an internal function that behaves like
> fdt_get_phandle() does today (returns the _actual_ phandle) and make
> fdt_get_phandle() only return valid phandles?

Eh.. "internal" has some fuzzy boundaries here.  Anything working with
overlay dtbs before they're applied to a base tree might see
unresolved phandle references.

> Alternatively, libfdt could provide to its clients a function similar to
> phandle_is_valid() (from dtc.h) and document in fdt_get_phandle() the need to
> validate its result through that function? That wouldn't fix existing (buggy)
> code but would at least make the API clearer.

That's not a bad idea.  I was mildly surprised we don't have something
like that already.

> Lastly, if none of the above can be implemented, the documentation of
> fdt_get_phandle() should at least reflect the fact that its result, even if
> non-zero, may not be a valid phandle.

Yes, we should definitely update that.

> 
> > 
> > > ---
> > >  libfdt/fdt_ro.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index 9f6c551..7d1da6d 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -507,6 +507,7 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
> > >  
> > >  uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
> > >  {
> > > +	uint32_t phandle;
> > >  	const fdt32_t *php;
> > >  	int len;
> > >  
> > > @@ -519,7 +520,11 @@ uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
> > >  			return 0;
> > >  	}
> > >  
> > > -	return fdt32_ld_(php);
> > > +	phandle = fdt32_ld_(php);
> > > +	if (phandle > (uint32_t)FDT_MAX_PHANDLE)
> > > +		phandle = 0;
> > > +
> > > +	return phandle;
> > >  }
> > >  
> > >  const char *fdt_get_alias_namelen(const void *fdt,
> > 
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]