[PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Pierre-Clément Tosi]

Make empty paths result in FDT_ERR_BADPATH.

Per the specification (v0.4-rc4):

> The convention for specifying a device path is:
>     /node-name-1/node-name-2/node-name-N
>
> The path to the root node is /.
>
> A unit address may be omitted if the full path to the
> node is unambiguous.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 libfdt/fdt_ro.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index c4c520c..46b4ef5 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -255,6 +255,9 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
 
 	FDT_RO_PROBE(fdt);
 
+	if (namelen < 1)
+		return -FDT_ERR_BADPATH;
+
 	/* see if we have an alias */
 	if (*path != '/') {
 		const char *q = memchr(path, '/', end - p);
-- 
2.42.0.609.gbb76f46606-goog


-- 
Pierre

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Simon Glass]

Hi Pierre-Clément,

On Fri, 6 Oct 2023 at 06:48, Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> Make empty paths result in FDT_ERR_BADPATH.
>
> Per the specification (v0.4-rc4):
>
> > The convention for specifying a device path is:
> >     /node-name-1/node-name-2/node-name-N
> >
> > The path to the root node is /.
> >
> > A unit address may be omitted if the full path to the
> > node is unambiguous.
>
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  libfdt/fdt_ro.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index c4c520c..46b4ef5 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -255,6 +255,9 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>
>         FDT_RO_PROBE(fdt);
>
> +       if (namelen < 1)
> +               return -FDT_ERR_BADPATH;
> +

This would be end == path, right? Would it be better to check that?
Are you worried about negative numbers?

>         /* see if we have an alias */
>         if (*path != '/') {
>                 const char *q = memchr(path, '/', end - p);
> --
> 2.42.0.609.gbb76f46606-goog
>
>
> --
> Pierre
>

Regards,
Simon

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Rob Herring]

On Fri, Oct 6, 2023 at 7:48 AM Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> Make empty paths result in FDT_ERR_BADPATH.
>
> Per the specification (v0.4-rc4):
>
> > The convention for specifying a device path is:
> >     /node-name-1/node-name-2/node-name-N
> >
> > The path to the root node is /.
> >
> > A unit address may be omitted if the full path to the
> > node is unambiguous.

How is this part relevant to this patch? In any case, I don't think we
actually allow that. Maybe libfdt does, but at least it's a dtc
warning.

Rob

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Simon Glass]

Hi Rob,

On Fri, 6 Oct 2023 at 08:06, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 6, 2023 at 7:48 AM Pierre-Clément Tosi <ptosi@google.com> wrote:
> >
> > Make empty paths result in FDT_ERR_BADPATH.
> >
> > Per the specification (v0.4-rc4):
> >
> > > The convention for specifying a device path is:
> > >     /node-name-1/node-name-2/node-name-N
> > >
> > > The path to the root node is /.
> > >
> > > A unit address may be omitted if the full path to the
> > > node is unambiguous.
>
> How is this part relevant to this patch? In any case, I don't think we
> actually allow that. Maybe libfdt does, but at least it's a dtc
> warning.

I agree it isn't relevant...re your point, see fdt_nodename_eq_()
which matches without the '@' so long as the length provided is the
length of the node name without the '@'. It doesn't check for
ambiguous nodes.

Regards,
Simon

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Pierre-Clément Tosi]

Hi Simon,

On Fri, Oct 06, 2023 at 07:22:14AM -0600, Simon Glass wrote:
> Hi Pierre-Clément,
> 
> On Fri, 6 Oct 2023 at 06:48, Pierre-Clément Tosi <ptosi@google.com> wrote:
> >
> > +       if (namelen < 1)
> > +               return -FDT_ERR_BADPATH;
> > +
> 
> This would be end == path, right? Would it be better to check that?
> Are you worried about negative numbers?
> 

The main focus of this patch was namelen==0 (e.g. fdt_path_offset(fdt, ""))
which violates the spec and makes the function return the offset of the root
node, a counter-intuitive result which, if used internally (other libfdt
functions call fdt_path_offset()), could lead to unintended behavior.

Furthermore, under the right conditions, fdt_path_offset(fdt, "") may lead to a
stack overflow attack, which this patch addresses but which is also separately
addressed by [1] (although one doesn't make the other redundant).

As I was adding a check on namelen, I took the opportunity to also reject all
negative values. Do you recommend I only reject end == path and accept/ignore
negative lengths?

If this validation makes sense, v2 will add coverage for it in 'make check'.

[1]: https://lore.kernel.org/devicetree-compiler/20231007110710.i2oj24oirdtyt5m4@google.com

> >         /* see if we have an alias */
> >         if (*path != '/') {
> >                 const char *q = memchr(path, '/', end - p);
> > --
> 
> Regards,
> Simon

Thanks,
-- 
Pierre

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[Pierre-Clément Tosi]

On Fri, Oct 06, 2023 at 04:42:36PM -0600, Simon Glass wrote:
> Hi Rob,
> 
> On Fri, 6 Oct 2023 at 08:06, Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 6, 2023 at 7:48 AM Pierre-Clément Tosi <ptosi@google.com> wrote:
> > >
> > > Make empty paths result in FDT_ERR_BADPATH.
> > >
> > > Per the specification (v0.4-rc4):
> > >
> > > > The convention for specifying a device path is:
> > > >     /node-name-1/node-name-2/node-name-N
> > > >
> > > > The path to the root node is /.
> > > >
> > > > A unit address may be omitted if the full path to the
> > > > node is unambiguous.
> >
> > How is this part relevant to this patch? In any case, I don't think we
> > actually allow that. Maybe libfdt does, but at least it's a dtc
> > warning.
> 
> I agree it isn't relevant...re your point, see fdt_nodename_eq_()
> which matches without the '@' so long as the length provided is the
> length of the node name without the '@'. It doesn't check for
> ambiguous nodes.
> 

Agreed, I was trying to be exhaustive: the irrelevant part won't be in v2.

> Regards,
> Simon

Thanks,
-- 
Pierre

Re: [PATCH] libfdt: fdt_path_offset_namelen: Reject empty path

[David Gibson]

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

On Fri, Oct 06, 2023 at 01:48:39PM +0100, Pierre-Clément Tosi wrote:
> Make empty paths result in FDT_ERR_BADPATH.
> 
> Per the specification (v0.4-rc4):
> 
> > The convention for specifying a device path is:
> >     /node-name-1/node-name-2/node-name-N
> >
> > The path to the root node is /.
> >
> > A unit address may be omitted if the full path to the
> > node is unambiguous.

As Rob noted, I don't really see how this quote is relevant to the
change at hand.

The change itself looks like a good idea, though.  Without this, we
will at the very least do a one byte bad access in the next line.  If
someone does path a negative value it will do... something bad,
probably.

> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  libfdt/fdt_ro.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index c4c520c..46b4ef5 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -255,6 +255,9 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
>  
>  	FDT_RO_PROBE(fdt);
>  
> +	if (namelen < 1)
> +		return -FDT_ERR_BADPATH;

It would be better to make this:
	if (!can_assume(VALID_INPUT) && namelen <= 0)

This allows the test to be optimised out in builds where we can assume
always valid parameters.

>  	/* see if we have an alias */
>  	if (*path != '/') {
>  		const char *q = memchr(path, '/', end - p);

It would also be really nice to add a testcase for behaviour in the
namelen == 0 and namelen < 0 cases.

-- 
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] libfdt: fdt_path_offset_namelen: Reject empty path

[David Gibson]

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

On Fri, Oct 06, 2023 at 07:22:14AM -0600, Simon Glass wrote:
> Hi Pierre-Clément,
> 
> On Fri, 6 Oct 2023 at 06:48, Pierre-Clément Tosi <ptosi@google.com> wrote:
> >
> > Make empty paths result in FDT_ERR_BADPATH.
> >
> > Per the specification (v0.4-rc4):
> >
> > > The convention for specifying a device path is:
> > >     /node-name-1/node-name-2/node-name-N
> > >
> > > The path to the root node is /.
> > >
> > > A unit address may be omitted if the full path to the
> > > node is unambiguous.
> >
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > ---
> >  libfdt/fdt_ro.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index c4c520c..46b4ef5 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -255,6 +255,9 @@ int fdt_path_offset_namelen(const void *fdt, const char *path, int namelen)
> >
> >         FDT_RO_PROBE(fdt);
> >
> > +       if (namelen < 1)
> > +               return -FDT_ERR_BADPATH;
> > +
> 
> This would be end == path, right? Would it be better to check that?

Not particularly, AFAICT.

> Are you worried about negative numbers?

If they're not, they probably should be..

> 
> >         /* see if we have an alias */
> >         if (*path != '/') {
> >                 const char *q = memchr(path, '/', end - p);
> >
> >
> >
> 
> Regards,
> Simon
> 

-- 
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 --]