Re: [PATCH v4 2/6] Add a way to control the level of checks in the code

Re: [PATCH v4 2/6] Add a way to control the level of checks in the code

[Simon Glass]

Hi David,

On Wed, 13 Nov 2019 at 14:54, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Add a new ASSUME_MASK option, which allows for some control over the
> checks used in libfdt. With all assumptions enabled, libfdt assumes that
> the input data and parameters are all correct and that internal errors
> cannot happen.
>
> By default no assumptions are made and all checks are enabled.
>
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>
> Changes in v4:
> - Add a can_assume() macros and squash the inline functions into one
> - Drop unnecessary FDT_ prefix
> - Fix 'Incosistencies' typo
> - Merge the 'friendly' and 'sane' checks into one
> - Update and expand comments
>
> Changes in v3:
> - Expand the comments about each check option
> - Invert the checks to be called assumptions
> - Move header-version code to fdt.c
> - Move the comments on check options to the enum
> - Use hex for CHK_MASK
>
> Changes in v2:
> - Add an fdt_ prefix to avoid namespace conflicts
> - Use symbolic names for _check functions and drop leading underscores
>
>  Makefile                 |  6 ++-
>  libfdt/libfdt_internal.h | 88 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+), 1 deletion(-)

Any thoughts on this latest series? We are still carrying a temporary
version in U-Boot.

Regards,
Simon

Re: [PATCH v4 1/6] libfdt: De-inline fdt_header_size()

[David Gibson]

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

On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > There does not seem to be a strong reason to inline this function.
> >
> > Well.. I think my rationale was just that this function should be a
> > single load, followed by the call to fdt_header_size_().  I suspect
> > it's not called enough for removing that to balance out the code
> > overhead of an extra function.  Or have you made measurements which
> > show I'm wrong about that?
> 
> Well it isn't called in U-Boot at all, other than via
> fdt_check_header(). The inlined version saves 16 bytes of code space
> on Thumb2.
> 
> But even better would be to drop fdt_check_header_(). something like:
> 
> size_t fdt_header_size(uint32_t version)
>    {
>    if (fdt_chk_version()) {
>       if (version <= 1)
>          return FDT_V1_SIZE;
>       else if (version <= 2)
>          return FDT_V2_SIZE;
>       else if (version <= 3)
>          return FDT_V3_SIZE;
>       else if (version <= 16)
>          return FDT_V16_SIZE;
>       }
> 
> return FDT_V17_SIZE;
> }
> 
> That saves an additional 8 bytes.

Uh.. I'm a little lost here.  Is what you're suggesting moving all of
fdt_check_header_() to the .h file, so it can be inlined (and
therefore constant folded)?

That could be a reasonable plan, though we'll need to make sure to
include an out-of-line version as well, for ABI compatibility.

> > > Also
> > > we are about to add some extra code to it which will increase its size.
> >
> > Will they, though?  As far as I can tell, the only changes you make to
> > this function in this series should be resolved at compile time.  So
> > that it should either be identical object code to what we have now, or
> > even simpler (in the assume case it will be returning a fixed value,
> > so in fact you'd probably get even better code size by allowing that
> > to be inlined).
> 
> Hmm yes that's true about it resolving at compile time.
> 
> But inlining a compile-time check presumably is no better than having
> it in a normal non-inlined function, since it is a compile-time check.
> For your second part, see the function I wrote above.
> 
> >
> > > Move it into fdt.c and use a simple declaration in libfdt.h
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > >
> > > Changes in v4:
> > > - Add fdt_header_size to version.lds
> > >
> > > Changes in v3:
> > > - Add a new patch to de-inline fdt_header_size()
> > >
> > > Changes in v2: None
> > >
> > >  libfdt/fdt.c       | 5 +++++
> > >  libfdt/libfdt.h    | 9 +++++----
> > >  libfdt/version.lds | 1 +
> > >  tests/testutils.c  | 1 -
> > >  4 files changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index d6ce7c0..3e37a4b 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -70,6 +70,11 @@ size_t fdt_header_size_(uint32_t version)
> > >               return FDT_V17_SIZE;
> > >  }
> > >
> > > +size_t fdt_header_size(const void *fdt)
> > > +{
> > > +     return fdt_header_size_(fdt_version(fdt));
> > > +}
> > > +
> > >  int fdt_check_header(const void *fdt)
> > >  {
> > >       size_t hdrsize;
> > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > index fc4c496..48f375c 100644
> > > --- a/libfdt/libfdt.h
> > > +++ b/libfdt/libfdt.h
> > > @@ -266,11 +266,12 @@ fdt_set_hdr_(size_dt_struct);
> > >   * fdt_header_size - return the size of the tree's header
> > >   * @fdt: pointer to a flattened device tree
> > >   */
> > > +size_t fdt_header_size(const void *fdt);
> > > +
> > > +/**
> > > + * fdt_header_size_ - internal function which takes a version number
> > > + */
> > >  size_t fdt_header_size_(uint32_t version);
> > > -static inline size_t fdt_header_size(const void *fdt)
> > > -{
> > > -     return fdt_header_size_(fdt_version(fdt));
> > > -}
> > >
> > >  /**
> > >   * fdt_check_header - sanity check a device tree header
> > > diff --git a/libfdt/version.lds b/libfdt/version.lds
> > > index ae32924..7ab85f1 100644
> > > --- a/libfdt/version.lds
> > > +++ b/libfdt/version.lds
> > > @@ -20,6 +20,7 @@ LIBFDT_1.2 {
> > >               fdt_get_alias_namelen;
> > >               fdt_get_alias;
> > >               fdt_get_path;
> > > +                fdt_header_size;
> > >               fdt_supernode_atdepth_offset;
> > >               fdt_node_depth;
> > >               fdt_parent_offset;
> > > diff --git a/tests/testutils.c b/tests/testutils.c
> > > index 5e494c5..53cb35f 100644
> > > --- a/tests/testutils.c
> > > +++ b/tests/testutils.c
> > > @@ -261,7 +261,6 @@ void vg_prepare_blob(void *fdt, size_t bufsize)
> > >
> > >       VALGRIND_MAKE_MEM_UNDEFINED(blob, bufsize);
> > >       VALGRIND_MAKE_MEM_DEFINED(blob, FDT_V1_SIZE);
> > > -     VALGRIND_MAKE_MEM_DEFINED(blob, fdt_header_size(fdt));
> > >
> > >       if (fdt_magic(fdt) == FDT_MAGIC) {
> > >               off_strings = fdt_off_dt_strings(fdt);
> >
> 
> BTW I didn't see any other comments on this series so though I might
> as well reply on just this one patch.

Sorry, I've just had real trouble making time for this amongst my day
job.

-- 
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 v4 2/6] Add a way to control the level of checks in the code

[David Gibson]

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

On Tue, Nov 12, 2019 at 06:54:18PM -0700, Simon Glass wrote:
> Add a new ASSUME_MASK option, which allows for some control over the
> checks used in libfdt. With all assumptions enabled, libfdt assumes that
> the input data and parameters are all correct and that internal errors
> cannot happen.
> 
> By default no assumptions are made and all checks are enabled.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v4:
> - Add a can_assume() macros and squash the inline functions into one
> - Drop unnecessary FDT_ prefix
> - Fix 'Incosistencies' typo
> - Merge the 'friendly' and 'sane' checks into one
> - Update and expand comments
> 
> Changes in v3:
> - Expand the comments about each check option
> - Invert the checks to be called assumptions
> - Move header-version code to fdt.c
> - Move the comments on check options to the enum
> - Use hex for CHK_MASK
> 
> Changes in v2:
> - Add an fdt_ prefix to avoid namespace conflicts
> - Use symbolic names for _check functions and drop leading underscores
> 
>  Makefile                 |  6 ++-
>  libfdt/libfdt_internal.h | 88 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 4d88157..bdeaf93 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -16,7 +16,11 @@ EXTRAVERSION =
>  LOCAL_VERSION =
>  CONFIG_LOCALVERSION =
>  
> -CPPFLAGS = -I libfdt -I .
> +# Control the assumptions made (e.g. risking security issues) in the code.
> +# See libfdt_internal.h for details
> +ASSUME_MASK ?= 0
> +
> +CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
>  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
>  	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
>  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 058c735..198480c 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -48,4 +48,92 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
>  
>  #define FDT_SW_MAGIC		(~FDT_MAGIC)
>  
> +/**********************************************************************/
> +/* Checking controls                                                  */
> +/**********************************************************************/
> +
> +#ifndef FDT_ASSUME_MASK
> +#define FDT_ASSUME_MASK 0
> +#endif
> +
> +/*
> + * Defines assumptions which can be enabled. Each of these can be enabled
> + * individually. For maximum saftey, don't enable any assumptions!
> + *
> + * For minimal code size and no safety, use ASSUME_PERFECT at your own risk.
> + * You should have another method of validating the device tree, such as a
> + * signature or hash check before using libfdt.
> + *
> + * For situations where security is not a concern it may be safe to enable
> + * ASSUME_SANE.
> + */
> +enum {
> +	/*
> +	 * This does essentially no checks. Only the latest device-tree
> +	 * version is correctly handled. Inconsistencies or errors in the device
> +	 * tree may cause undefined behaviour or crashes.

Likewise for errors in parameters, yes?

> +	 * If an error occurs when modifying the tree it may leave the tree in
> +	 * an intermediate (but valid) state. As an example, adding a property
> +	 * where there is insufficient space may result in the property name
> +	 * being added to the string table even though the property itself is
> +	 * not added to the struct section.
> +	 *
> +	 * Only use this if you have a fully validated device tree with
> +	 * the latest supported version and wish to minimise code size.
> +	 */
> +	ASSUME_PERFECT		= 0xff,
> +
> +	/*
> +	 * This assumes that the device tree is sane. i.e. header metadata
> +	 * and basic hierarchy are correct.
> +	 *
> +	 * It also assumes libfdt functions are called with valid parameters,
> +	 * i.e. not trigger FDT_ERR_BADOFFSET or offsets that are out of bounds.
> +	 * It disables any extensive checking of parameters and the device
> +	 * tree, making various assumptions about correctness. Normal device
> +	 * trees produced by libfdt and the compiler should be handled safely.
> +	 * Malicious device trees and complete garbage may cause libfdt to
> +	 * behave badly or crash.
> +	 *
> +	 * These checks will be sufficient if you have a valid device tree with
> +	 * no internal inconsistencies and call libfdt with correct parameters.
> +	 * With this assumption, libfdt will generally not return
> +	 * -FDT_ERR_INTERNAL, -FDT_ERR_BADLAYOUT, etc.
> +	 */
> +	ASSUME_SANE		= 1 << 0,

How tricky would it be to split this into ASSUME_VALID_DTB and
ASSUME_VALID_PARAMETERS?  I don't know useful those are on their own,
but it seems to me it might be easier to define the effect of this if
split that way.

> +
> +	/*
> +	 * This disables checks for device-tree version and removes all code
> +	 * which handles older versions.
> +	 *
> +	 * Only enable this if you know you have a device tree with the latest
> +	 * version.
> +	 */
> +	ASSUME_LATEST		= 1 << 1,
> +
> +	/*
> +	 * This assume that it is OK for a failed additional to the device tree
> +	 * due to lack of space or some other problem can skip any rollback
> +	 * steps (such as dropping the property name from the string table).
> +	 * This is safe to enable in most circumstances, even though it may
> +	 * leave the tree in a sub-optimal state.
> +	 */
> +	ASSUME_NO_ROLLBACK	= 1 << 2,
> +};
> +
> +/**
> + * _can_assume() - check if a particular assumption is enabled

Don't use a leading _ please, that's reserved for system libraries.
I've moved to trailing _ for that reason.

> + *
> + * @mask: Mask to check (ASSUME_...)
> + * @return true if that assumption is enabled, else false
> + */
> +static inline bool _can_assume(int mask)
> +{
> +	return FDT_ASSUME_MASK & mask;
> +}
> +
> +/** helper macros for checking assumptions */
> +#define can_assume(_assume)	_can_assume(ASSUME_ ## _assume)
> +
>  #endif /* LIBFDT_INTERNAL_H */

-- 
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 v4 3/6] libfdt: Add support for disabling sanity checks

[David Gibson]

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

On Tue, Nov 12, 2019 at 06:54:19PM -0700, Simon Glass wrote:
> Allow enabling FDT_ASSUME_SANE to disable sanity checks.

LGTM, with the exception of the possible splitting of assuming sane
dtb contents from assuming sane parameters mentioned earlier.

> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt.c             | 62 +++++++++++++++++++---------------
>  libfdt/fdt_ro.c          | 73 ++++++++++++++++++++++++++--------------
>  libfdt/fdt_rw.c          |  6 +++-
>  libfdt/fdt_sw.c          | 29 ++++++++++------
>  libfdt/libfdt_internal.h |  9 +++--
>  5 files changed, 113 insertions(+), 66 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 3e37a4b..d89c0e0 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
>  
>  	if (fdt_magic(fdt) != FDT_MAGIC)
>  		return -FDT_ERR_BADMAGIC;
> -	hdrsize = fdt_header_size(fdt);
>  	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>  	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
>  		return -FDT_ERR_BADVERSION;
>  	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
>  		return -FDT_ERR_BADVERSION;
> +	hdrsize = fdt_header_size(fdt);
> +	if (!can_assume(SANE)) {
>  
> -	if ((fdt_totalsize(fdt) < hdrsize)
> -	    || (fdt_totalsize(fdt) > INT_MAX))
> -		return -FDT_ERR_TRUNCATED;
> -
> -	/* Bounds check memrsv block */
> -	if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> -		return -FDT_ERR_TRUNCATED;
> +		if ((fdt_totalsize(fdt) < hdrsize)
> +		    || (fdt_totalsize(fdt) > INT_MAX))
> +			return -FDT_ERR_TRUNCATED;
>  
> -	/* Bounds check structure block */
> -	if (fdt_version(fdt) < 17) {
> +		/* Bounds check memrsv block */
>  		if (!check_off_(hdrsize, fdt_totalsize(fdt),
> -				fdt_off_dt_struct(fdt)))
> +				fdt_off_mem_rsvmap(fdt)))
>  			return -FDT_ERR_TRUNCATED;
> -	} else {
> +	}
> +
> +	if (!can_assume(SANE)) {
> +		/* Bounds check structure block */
> +		if (fdt_version(fdt) < 17) {
> +			if (!check_off_(hdrsize, fdt_totalsize(fdt),
> +					fdt_off_dt_struct(fdt)))
> +				return -FDT_ERR_TRUNCATED;
> +		} else {
> +			if (!check_block_(hdrsize, fdt_totalsize(fdt),
> +					  fdt_off_dt_struct(fdt),
> +					  fdt_size_dt_struct(fdt)))
> +				return -FDT_ERR_TRUNCATED;
> +		}
> +
> +		/* Bounds check strings block */
>  		if (!check_block_(hdrsize, fdt_totalsize(fdt),
> -				  fdt_off_dt_struct(fdt),
> -				  fdt_size_dt_struct(fdt)))
> +				  fdt_off_dt_strings(fdt),
> +				  fdt_size_dt_strings(fdt)))
>  			return -FDT_ERR_TRUNCATED;
>  	}
>  
> -	/* Bounds check strings block */
> -	if (!check_block_(hdrsize, fdt_totalsize(fdt),
> -			  fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> -		return -FDT_ERR_TRUNCATED;
> -
>  	return 0;
>  }
>  
> @@ -120,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  {
>  	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
>  
> -	if ((absoffset < offset)
> -	    || ((absoffset + len) < absoffset)
> -	    || (absoffset + len) > fdt_totalsize(fdt))
> -		return NULL;
> +	if (!can_assume(SANE))
> +		if ((absoffset < offset)
> +		    || ((absoffset + len) < absoffset)
> +		    || (absoffset + len) > fdt_totalsize(fdt))
> +			return NULL;
>  
>  	if (fdt_version(fdt) >= 0x11)
>  		if (((offset + len) < offset)
> @@ -142,7 +149,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  
>  	*nextoffset = -FDT_ERR_TRUNCATED;
>  	tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> -	if (!tagp)
> +	if (!can_assume(SANE) && !tagp)
>  		return FDT_END; /* premature end */
>  	tag = fdt32_to_cpu(*tagp);
>  	offset += FDT_TAGSIZE;
> @@ -154,13 +161,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		do {
>  			p = fdt_offset_ptr(fdt, offset++, 1);
>  		} while (p && (*p != '\0'));
> -		if (!p)
> +		if (!can_assume(SANE) && !p)
>  			return FDT_END; /* premature end */
>  		break;
>  
>  	case FDT_PROP:
>  		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> -		if (!lenp)
> +		if (!can_assume(SANE) && !lenp)
>  			return FDT_END; /* premature end */
>  		/* skip-name offset, length and value */
>  		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> @@ -179,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		return FDT_END;
>  	}
>  
> -	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> +	if (!can_assume(SANE) &&
> +	    !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
>  		return FDT_END; /* premature end */
>  
>  	*nextoffset = FDT_TAGALIGN(offset);
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index a5c2797..5615bff 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
>  	int olen;
>  	const char *p = fdt_get_name(fdt, offset, &olen);
>  
> -	if (!p || olen < len)
> +	if (!p || (!can_assume(SANE) && olen < len))
>  		/* short match */
>  		return 0;
>  
> @@ -33,17 +33,26 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
>  
>  const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  {
> -	int32_t totalsize = fdt_ro_probe_(fdt);
> -	uint32_t absoffset = stroffset + fdt_off_dt_strings(fdt);
> +	int32_t totalsize;
> +	uint32_t absoffset;
>  	size_t len;
>  	int err;
>  	const char *s, *n;
>  
> +	if (can_assume(SANE)) {
> +		s = (const char *)fdt + fdt_off_dt_strings(fdt) + stroffset;
> +
> +		if (lenp)
> +			*lenp = strlen(s);
> +		return s;
> +	}
> +	totalsize = fdt_ro_probe_(fdt);
>  	err = totalsize;
>  	if (totalsize < 0)
>  		goto fail;
>  
>  	err = -FDT_ERR_BADOFFSET;
> +	absoffset = stroffset + fdt_off_dt_strings(fdt);
>  	if (absoffset >= totalsize)
>  		goto fail;
>  	len = totalsize - absoffset;
> @@ -151,10 +160,13 @@ static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
>  	int offset = n * sizeof(struct fdt_reserve_entry);
>  	int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
>  
> -	if (absoffset < fdt_off_mem_rsvmap(fdt))
> -		return NULL;
> -	if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
> -		return NULL;
> +	if (!can_assume(SANE)) {
> +		if (absoffset < fdt_off_mem_rsvmap(fdt))
> +			return NULL;
> +		if (absoffset > fdt_totalsize(fdt) -
> +		    sizeof(struct fdt_reserve_entry))
> +			return NULL;
> +	}
>  	return fdt_mem_rsv_(fdt, n);
>  }
>  
> @@ -164,7 +176,7 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>  
>  	FDT_RO_PROBE(fdt);
>  	re = fdt_mem_rsv(fdt, n);
> -	if (!re)
> +	if (!can_assume(SANE) && !re)
>  		return -FDT_ERR_BADOFFSET;
>  
>  	*address = fdt64_ld(&re->address);
> @@ -289,9 +301,10 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  	const char *nameptr;
>  	int err;
>  
> -	if (((err = fdt_ro_probe_(fdt)) < 0)
> -	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> -			goto fail;
> +	if (!can_assume(SANE) &&
> +	    (((err = fdt_ro_probe_(fdt)) < 0)
> +	     || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)))
> +		goto fail;
>  
>  	nameptr = nh->name;
>  
> @@ -346,7 +359,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
>  	int err;
>  	const struct fdt_property *prop;
>  
> -	if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> +	if (!can_assume(SANE) &&
> +	    (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
>  		if (lenp)
>  			*lenp = err;
>  		return NULL;
> @@ -388,7 +402,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
>  	     (offset = fdt_next_property_offset(fdt, offset))) {
>  		const struct fdt_property *prop;
>  
> -		if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> +		prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> +		if (!can_assume(SANE) && !prop) {
>  			offset = -FDT_ERR_INTERNAL;
>  			break;
>  		}
> @@ -461,14 +476,19 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  	if (namep) {
>  		const char *name;
>  		int namelen;
> -		name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
> -				      &namelen);
> -		if (!name) {
> -			if (lenp)
> -				*lenp = namelen;
> -			return NULL;
> +
> +		if (!can_assume(SANE)) {
> +			name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
> +					      &namelen);
> +			if (!name) {
> +				if (lenp)
> +					*lenp = namelen;
> +				return NULL;
> +			}
> +			*namep = name;
> +		} else {
> +			*namep = fdt_string(fdt, fdt32_ld(&prop->nameoff));
>  		}
> -		*namep = name;
>  	}
>  
>  	/* Handle realignment */
> @@ -598,10 +618,12 @@ int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
>  		}
>  	}
>  
> -	if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
> -		return -FDT_ERR_BADOFFSET;
> -	else if (offset == -FDT_ERR_BADOFFSET)
> -		return -FDT_ERR_BADSTRUCTURE;
> +	if (!can_assume(SANE)) {
> +		if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
> +			return -FDT_ERR_BADOFFSET;
> +		else if (offset == -FDT_ERR_BADOFFSET)
> +			return -FDT_ERR_BADSTRUCTURE;
> +	}
>  
>  	return offset; /* error from fdt_next_node() */
>  }
> @@ -613,7 +635,8 @@ int fdt_node_depth(const void *fdt, int nodeoffset)
>  
>  	err = fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, &nodedepth);
>  	if (err)
> -		return (err < 0) ? err : -FDT_ERR_INTERNAL;
> +		return (can_assume(SANE) || err < 0) ? err :
> +			-FDT_ERR_INTERNAL;
>  	return nodedepth;
>  }
>  
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 8795947..1dd9cf0 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -13,6 +13,8 @@
>  static int fdt_blocks_misordered_(const void *fdt,
>  				  int mem_rsv_size, int struct_size)
>  {
> +	if (can_assume(SANE))
> +		return false;
>  	return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
>  		|| (fdt_off_dt_struct(fdt) <
>  		    (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
>  
>  static int fdt_rw_probe_(void *fdt)
>  {
> +	if (can_assume(SANE))
> +		return 0;
>  	FDT_RO_PROBE(fdt);
>  
>  	if (fdt_version(fdt) < 17)
> @@ -40,7 +44,7 @@ static int fdt_rw_probe_(void *fdt)
>  #define FDT_RW_PROBE(fdt) \
>  	{ \
>  		int err_; \
> -		if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> +		if (!can_assume(SANE) && (err_ = fdt_rw_probe_(fdt)) != 0) \
>  			return err_; \
>  	}
>  
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 76bea22..cd0032a 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -12,17 +12,20 @@
>  
>  static int fdt_sw_probe_(void *fdt)
>  {
> -	if (fdt_magic(fdt) == FDT_MAGIC)
> -		return -FDT_ERR_BADSTATE;
> -	else if (fdt_magic(fdt) != FDT_SW_MAGIC)
> -		return -FDT_ERR_BADMAGIC;
> +	if (!can_assume(SANE)) {
> +		if (fdt_magic(fdt) == FDT_MAGIC)
> +			return -FDT_ERR_BADSTATE;
> +		else if (fdt_magic(fdt) != FDT_SW_MAGIC)
> +			return -FDT_ERR_BADMAGIC;
> +	}
> +
>  	return 0;
>  }
>  
>  #define FDT_SW_PROBE(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_(fdt)) != 0) \
> +		if (!can_assume(SANE) && (err = fdt_sw_probe_(fdt)) != 0) \
>  			return err; \
>  	}
>  
> @@ -38,7 +41,7 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>  	if (err)
>  		return err;
>  
> -	if (fdt_off_dt_strings(fdt) != 0)
> +	if (!can_assume(SANE) && fdt_off_dt_strings(fdt) != 0)
>  		return -FDT_ERR_BADSTATE;
>  	return 0;
>  }
> @@ -46,7 +49,8 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>  #define FDT_SW_PROBE_MEMRSV(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_memrsv_(fdt)) != 0) \
> +		if (!can_assume(SANE) && \
> +		    (err = fdt_sw_probe_memrsv_(fdt)) != 0) \
>  			return err; \
>  	}
>  
> @@ -60,7 +64,11 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>   */
>  static int fdt_sw_probe_struct_(void *fdt)
>  {
> -	int err = fdt_sw_probe_(fdt);
> +	int err;
> +
> +	if (can_assume(SANE))
> +		return 0;
> +	err = fdt_sw_probe_(fdt);
>  	if (err)
>  		return err;
>  
> @@ -72,7 +80,8 @@ static int fdt_sw_probe_struct_(void *fdt)
>  #define FDT_SW_PROBE_STRUCT(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_struct_(fdt)) != 0) \
> +		if (!can_assume(SANE) && \
> +		    (err = fdt_sw_probe_struct_(fdt)) != 0) \
>  			return err; \
>  	}
>  
> @@ -151,7 +160,7 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
>  	headsize = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>  	tailsize = fdt_size_dt_strings(fdt);
>  
> -	if ((headsize + tailsize) > fdt_totalsize(fdt))
> +	if (!can_assume(SANE) && (headsize + tailsize) > fdt_totalsize(fdt))
>  		return -FDT_ERR_INTERNAL;
>  
>  	if ((headsize + tailsize) > bufsize)
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 198480c..4612fd0 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -13,9 +13,12 @@
>  int32_t fdt_ro_probe_(const void *fdt);
>  #define FDT_RO_PROBE(fdt)					\
>  	{							\
> -		int32_t totalsize_;				\
> -		if ((totalsize_ = fdt_ro_probe_(fdt)) < 0)	\
> -			return totalsize_;			\
> +		int32_t totalsize_;					\
> +		if (!can_assume(SANE)) {				\
> +			totalsize_ = fdt_ro_probe_(fdt);	\
> +			if (totalsize_ < 0)			\
> +				return totalsize_;		\
> +		}						\
>  	}
>  
>  int fdt_check_node_offset_(const void *fdt, int offset);

-- 
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 v4 4/6] libfdt: Add support for disabling rollback handling

[David Gibson]

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

On Tue, Nov 12, 2019 at 06:54:20PM -0700, Simon Glass wrote:
> Allow enabling FDT_ASSUME_NO_ROLLBACK to disable rolling back after a
> failed operation.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt_rw.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 1dd9cf0..fe61e63 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -116,6 +116,15 @@ static int fdt_splice_string_(void *fdt, int newlen)
>  	return 0;
>  }
>  
> +/**
> + * fdt_find_add_string_() - Find or allocate a string
> + *
> + * @fdt: pointer to the device tree to check/adjust
> + * @s: string to find/add
> + * @allocated: Set to 0 if the string was found, 1 if not found and so
> + *	allocated. Ignored if can_assume(NO_ROLLBACK)
> + * @return offset of string in the string table (whether found or added)
> + */
>  static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
>  {
>  	char *strtab = (char *)fdt + fdt_off_dt_strings(fdt);
> @@ -124,7 +133,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
>  	int len = strlen(s) + 1;
>  	int err;
>  
> -	*allocated = 0;
> +	if (!can_assume(NO_ROLLBACK))
> +		*allocated = 0;
>  
>  	p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
>  	if (p)
> @@ -136,7 +146,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
>  	if (err)
>  		return err;
>  
> -	*allocated = 1;
> +	if (!can_assume(NO_ROLLBACK))
> +		*allocated = 1;
>  
>  	memcpy(new, s, len);
>  	return (new - strtab);
> @@ -210,7 +221,8 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
>  
>  	err = fdt_splice_struct_(fdt, *prop, 0, proplen);
>  	if (err) {
> -		if (allocated)
> +		/* Delete the string if we failed to add it */
> +		if (!can_assume(NO_ROLLBACK) && allocated)
>  			fdt_del_last_string_(fdt, name);
>  		return err;
>  	}

-- 
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 v4 5/6] libfdt: Add support for disabling version checks

[David Gibson]

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

On Tue, Nov 12, 2019 at 06:54:21PM -0700, Simon Glass wrote:
> Allow enabling FDT_ASSUME_LATEST to disable version checks.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

LGTM, one minor comment below.

> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt.c    | 34 +++++++++++++++++++++-------------
>  libfdt/fdt_ro.c | 16 ++++++++--------
>  libfdt/fdt_rw.c |  6 +++---
>  3 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index d89c0e0..78dbe58 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
>  
>  	if (fdt_magic(fdt) == FDT_MAGIC) {
>  		/* Complete tree */
> -		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> -			return -FDT_ERR_BADVERSION;
> -		if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
> -			return -FDT_ERR_BADVERSION;
> +		if (!can_assume(LATEST)) {
> +			if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> +				return -FDT_ERR_BADVERSION;
> +			if (fdt_last_comp_version(fdt) >
> +					FDT_LAST_SUPPORTED_VERSION)
> +				return -FDT_ERR_BADVERSION;
> +		}
>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
>  		/* Unfinished sequential-write blob */
>  		if (fdt_size_dt_struct(fdt) == 0)
> @@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
>  
>  size_t fdt_header_size(const void *fdt)
>  {
> -	return fdt_header_size_(fdt_version(fdt));
> +	return can_assume(LATEST) ? FDT_V17_SIZE :
> +		fdt_header_size_(fdt_version(fdt));
>  }
>  
>  int fdt_check_header(const void *fdt)
> @@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
>  
>  	if (fdt_magic(fdt) != FDT_MAGIC)
>  		return -FDT_ERR_BADMAGIC;
> -	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> -	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> -		return -FDT_ERR_BADVERSION;
> -	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> -		return -FDT_ERR_BADVERSION;
> +	if (!can_assume(LATEST)) {
> +		if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> +		    || (fdt_last_comp_version(fdt) >
> +			FDT_LAST_SUPPORTED_VERSION))
> +			return -FDT_ERR_BADVERSION;
> +		if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> +			return -FDT_ERR_BADVERSION;
> +	}
>  	hdrsize = fdt_header_size(fdt);
>  	if (!can_assume(SANE)) {
>  
> @@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
>  
>  	if (!can_assume(SANE)) {
>  		/* Bounds check structure block */
> -		if (fdt_version(fdt) < 17) {
> +		if (!can_assume(LATEST) && fdt_version(fdt) < 17) {

I wonder if we should make fdt_version() return 17 unconditionally in
the can_assume() case, which might make some of these tests redundant.

>  			if (!check_off_(hdrsize, fdt_totalsize(fdt),
>  					fdt_off_dt_struct(fdt)))
>  				return -FDT_ERR_TRUNCATED;
> @@ -132,7 +139,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  		    || (absoffset + len) > fdt_totalsize(fdt))
>  			return NULL;
>  
> -	if (fdt_version(fdt) >= 0x11)
> +	if (can_assume(LATEST) || fdt_version(fdt) >= 0x11)
>  		if (((offset + len) < offset)
>  		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>  			return NULL;
> @@ -172,7 +179,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		/* skip-name offset, length and value */
>  		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>  			+ fdt32_to_cpu(*lenp);
> -		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> +		if (!can_assume(LATEST) &&
> +		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
>  		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
>  			offset += 4;
>  		break;
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 5615bff..13b63c3 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -60,7 +60,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  	if (fdt_magic(fdt) == FDT_MAGIC) {
>  		if (stroffset < 0)
>  			goto fail;
> -		if (fdt_version(fdt) >= 17) {
> +		if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  			if (stroffset >= fdt_size_dt_strings(fdt))
>  				goto fail;
>  			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
> @@ -308,7 +308,7 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  
>  	nameptr = nh->name;
>  
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		/*
>  		 * For old FDT versions, match the naming conventions of V16:
>  		 * give only the leaf name (after all /). The actual tree
> @@ -381,7 +381,7 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>  	/* Prior to version 16, properties may need realignment
>  	 * and this API does not work. fdt_getprop_*() will, however. */
>  
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		if (lenp)
>  			*lenp = -FDT_ERR_BADVERSION;
>  		return NULL;
> @@ -428,7 +428,7 @@ const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>  {
>  	/* Prior to version 16, properties may need realignment
>  	 * and this API does not work. fdt_getprop_*() will, however. */
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		if (lenp)
>  			*lenp = -FDT_ERR_BADVERSION;
>  		return NULL;
> @@ -459,8 +459,8 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  		return NULL;
>  
>  	/* Handle realignment */
> -	if (fdt_version(fdt) < 0x10 && (poffset + sizeof(*prop)) % 8 &&
> -	    fdt32_ld(&prop->len) >= 8)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> +	    (poffset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> @@ -492,8 +492,8 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  	}
>  
>  	/* Handle realignment */
> -	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> -	    fdt32_ld(&prop->len) >= 8)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> +	    (offset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index fe61e63..4ff7942 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -30,12 +30,12 @@ static int fdt_rw_probe_(void *fdt)
>  		return 0;
>  	FDT_RO_PROBE(fdt);
>  
> -	if (fdt_version(fdt) < 17)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 17)
>  		return -FDT_ERR_BADVERSION;
>  	if (fdt_blocks_misordered_(fdt, sizeof(struct fdt_reserve_entry),
>  				   fdt_size_dt_struct(fdt)))
>  		return -FDT_ERR_BADLAYOUT;
> -	if (fdt_version(fdt) > 17)
> +	if (!can_assume(LATEST) && fdt_version(fdt) > 17)
>  		fdt_set_version(fdt, 17);
>  
>  	return 0;
> @@ -427,7 +427,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  	mem_rsv_size = (fdt_num_mem_rsv(fdt)+1)
>  		* sizeof(struct fdt_reserve_entry);
>  
> -	if (fdt_version(fdt) >= 17) {
> +	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  		struct_size = fdt_size_dt_struct(fdt);
>  	} else {
>  		struct_size = 0;

-- 
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 v4 6/6] libfdt: Allow exclusion of fdt_check_full()

[David Gibson]

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

On Tue, Nov 12, 2019 at 06:54:22PM -0700, Simon Glass wrote:
> This function is used to perform a full check of the device tree. Allow
> it to be excluded if all assumptions are enabled.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
> 
> Changes in v4: None
> Changes in v3:
> - Fix 'santiy' typo
> - Instead of excluding fdt_check_full() put it in its own file
> - Rearrange code in terms of checks instead of files changed, to aid review
> - Replace 'unsigned' with 'unsigned int'
> - Update commit message a little
> 
> Changes in v2:
> - Add a comment to fdt_find_add_string_()
> - Correct inverted version checks in a few cases
> - Drop optimised code path in fdt_nodename_eq_()
> - Update to use new check functions
> - Use fdt_chk_base() in fdt_blocks_misordered_()
> 
>  libfdt/Makefile.libfdt |  2 +-
>  libfdt/fdt_check.c     | 74 ++++++++++++++++++++++++++++++++++++++++++
>  libfdt/fdt_ro.c        | 63 -----------------------------------
>  3 files changed, 75 insertions(+), 64 deletions(-)
>  create mode 100644 libfdt/fdt_check.c
> 
> diff --git a/libfdt/Makefile.libfdt b/libfdt/Makefile.libfdt
> index e546397..b6d8fc0 100644
> --- a/libfdt/Makefile.libfdt
> +++ b/libfdt/Makefile.libfdt
> @@ -8,7 +8,7 @@ LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
>  LIBFDT_INCLUDES = fdt.h libfdt.h libfdt_env.h
>  LIBFDT_VERSION = version.lds
>  LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c \
> -	fdt_addresses.c fdt_overlay.c
> +	fdt_addresses.c fdt_overlay.c fdt_check.c
>  LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
>  LIBFDT_LIB = libfdt-$(DTC_VERSION).$(SHAREDLIB_EXT)
>  
> diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
> new file mode 100644
> index 0000000..7f6a96c
> --- /dev/null
> +++ b/libfdt/fdt_check.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
> +/*
> + * libfdt - Flat Device Tree manipulation
> + * Copyright (C) 2006 David Gibson, IBM Corporation.
> + */
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +int fdt_check_full(const void *fdt, size_t bufsize)
> +{
> +	int err;
> +	int num_memrsv;
> +	int offset, nextoffset = 0;
> +	uint32_t tag;
> +	unsigned int depth = 0;
> +	const void *prop;
> +	const char *propname;
> +
> +	if (bufsize < FDT_V1_SIZE)
> +		return -FDT_ERR_TRUNCATED;
> +	err = fdt_check_header(fdt);
> +	if (err != 0)
> +		return err;
> +	if (bufsize < fdt_totalsize(fdt))
> +		return -FDT_ERR_TRUNCATED;
> +
> +	num_memrsv = fdt_num_mem_rsv(fdt);
> +	if (num_memrsv < 0)
> +		return num_memrsv;
> +
> +	while (1) {
> +		offset = nextoffset;
> +		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +
> +		if (nextoffset < 0)
> +			return nextoffset;
> +
> +		switch (tag) {
> +		case FDT_NOP:
> +			break;
> +
> +		case FDT_END:
> +			if (depth != 0)
> +				return -FDT_ERR_BADSTRUCTURE;
> +			return 0;
> +
> +		case FDT_BEGIN_NODE:
> +			depth++;
> +			if (depth > INT_MAX)
> +				return -FDT_ERR_BADSTRUCTURE;
> +			break;
> +
> +		case FDT_END_NODE:
> +			if (depth == 0)
> +				return -FDT_ERR_BADSTRUCTURE;
> +			depth--;
> +			break;
> +
> +		case FDT_PROP:
> +			prop = fdt_getprop_by_offset(fdt, offset, &propname,
> +						     &err);
> +			if (!prop)
> +				return err;
> +			break;
> +
> +		default:
> +			return -FDT_ERR_INTERNAL;
> +		}
> +	}
> +}
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 13b63c3..cbb30c8 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -856,66 +856,3 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
>  
>  	return offset; /* error from fdt_next_node() */
>  }
> -
> -int fdt_check_full(const void *fdt, size_t bufsize)
> -{
> -	int err;
> -	int num_memrsv;
> -	int offset, nextoffset = 0;
> -	uint32_t tag;
> -	unsigned depth = 0;
> -	const void *prop;
> -	const char *propname;
> -
> -	if (bufsize < FDT_V1_SIZE)
> -		return -FDT_ERR_TRUNCATED;
> -	err = fdt_check_header(fdt);
> -	if (err != 0)
> -		return err;
> -	if (bufsize < fdt_totalsize(fdt))
> -		return -FDT_ERR_TRUNCATED;
> -
> -	num_memrsv = fdt_num_mem_rsv(fdt);
> -	if (num_memrsv < 0)
> -		return num_memrsv;
> -
> -	while (1) {
> -		offset = nextoffset;
> -		tag = fdt_next_tag(fdt, offset, &nextoffset);
> -
> -		if (nextoffset < 0)
> -			return nextoffset;
> -
> -		switch (tag) {
> -		case FDT_NOP:
> -			break;
> -
> -		case FDT_END:
> -			if (depth != 0)
> -				return -FDT_ERR_BADSTRUCTURE;
> -			return 0;
> -
> -		case FDT_BEGIN_NODE:
> -			depth++;
> -			if (depth > INT_MAX)
> -				return -FDT_ERR_BADSTRUCTURE;
> -			break;
> -
> -		case FDT_END_NODE:
> -			if (depth == 0)
> -				return -FDT_ERR_BADSTRUCTURE;
> -			depth--;
> -			break;
> -
> -		case FDT_PROP:
> -			prop = fdt_getprop_by_offset(fdt, offset, &propname,
> -						     &err);
> -			if (!prop)
> -				return err;
> -			break;
> -
> -		default:
> -			return -FDT_ERR_INTERNAL;
> -		}
> -	}
> -}

-- 
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 v4 1/6] libfdt: De-inline fdt_header_size()

[Simon Glass]

Hi David,

On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > There does not seem to be a strong reason to inline this function.
> > >
> > > Well.. I think my rationale was just that this function should be a
> > > single load, followed by the call to fdt_header_size_().  I suspect
> > > it's not called enough for removing that to balance out the code
> > > overhead of an extra function.  Or have you made measurements which
> > > show I'm wrong about that?
> >
> > Well it isn't called in U-Boot at all, other than via
> > fdt_check_header(). The inlined version saves 16 bytes of code space
> > on Thumb2.
> >
> > But even better would be to drop fdt_check_header_(). something like:
> >
> > size_t fdt_header_size(uint32_t version)
> >    {
> >    if (fdt_chk_version()) {
> >       if (version <= 1)
> >          return FDT_V1_SIZE;
> >       else if (version <= 2)
> >          return FDT_V2_SIZE;
> >       else if (version <= 3)
> >          return FDT_V3_SIZE;
> >       else if (version <= 16)
> >          return FDT_V16_SIZE;
> >       }
> >
> > return FDT_V17_SIZE;
> > }
> >
> > That saves an additional 8 bytes.
>
> Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> fdt_check_header_() to the .h file, so it can be inlined (and
> therefore constant folded)?
>
> That could be a reasonable plan, though we'll need to make sure to
> include an out-of-line version as well, for ABI compatibility.

OK that sounds like a pain with little gain. Let's leave it as it is then.

Regards,
Simon

[..]

Re: [PATCH v4 2/6] Add a way to control the level of checks in the code

[Simon Glass]

Hi David,

On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Nov 12, 2019 at 06:54:18PM -0700, Simon Glass wrote:
> > Add a new ASSUME_MASK option, which allows for some control over the
> > checks used in libfdt. With all assumptions enabled, libfdt assumes that
> > the input data and parameters are all correct and that internal errors
> > cannot happen.
> >
> > By default no assumptions are made and all checks are enabled.
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> >
> > Changes in v4:
> > - Add a can_assume() macros and squash the inline functions into one
> > - Drop unnecessary FDT_ prefix
> > - Fix 'Incosistencies' typo
> > - Merge the 'friendly' and 'sane' checks into one
> > - Update and expand comments
> >
> > Changes in v3:
> > - Expand the comments about each check option
> > - Invert the checks to be called assumptions
> > - Move header-version code to fdt.c
> > - Move the comments on check options to the enum
> > - Use hex for CHK_MASK
> >
> > Changes in v2:
> > - Add an fdt_ prefix to avoid namespace conflicts
> > - Use symbolic names for _check functions and drop leading underscores
> >
> >  Makefile                 |  6 ++-
> >  libfdt/libfdt_internal.h | 88 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 93 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 4d88157..bdeaf93 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -16,7 +16,11 @@ EXTRAVERSION =
> >  LOCAL_VERSION =
> >  CONFIG_LOCALVERSION =
> >
> > -CPPFLAGS = -I libfdt -I .
> > +# Control the assumptions made (e.g. risking security issues) in the code.
> > +# See libfdt_internal.h for details
> > +ASSUME_MASK ?= 0
> > +
> > +CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> >  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
> >       -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> >  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
> > diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> > index 058c735..198480c 100644
> > --- a/libfdt/libfdt_internal.h
> > +++ b/libfdt/libfdt_internal.h
> > @@ -48,4 +48,92 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
> >
> >  #define FDT_SW_MAGIC         (~FDT_MAGIC)
> >
> > +/**********************************************************************/
> > +/* Checking controls                                                  */
> > +/**********************************************************************/
> > +
> > +#ifndef FDT_ASSUME_MASK
> > +#define FDT_ASSUME_MASK 0
> > +#endif
> > +
> > +/*
> > + * Defines assumptions which can be enabled. Each of these can be enabled
> > + * individually. For maximum saftey, don't enable any assumptions!
> > + *
> > + * For minimal code size and no safety, use ASSUME_PERFECT at your own risk.
> > + * You should have another method of validating the device tree, such as a
> > + * signature or hash check before using libfdt.
> > + *
> > + * For situations where security is not a concern it may be safe to enable
> > + * ASSUME_SANE.
> > + */
> > +enum {
> > +     /*
> > +      * This does essentially no checks. Only the latest device-tree
> > +      * version is correctly handled. Inconsistencies or errors in the device
> > +      * tree may cause undefined behaviour or crashes.
>
> Likewise for errors in parameters, yes?

Will add

>
> > +      * If an error occurs when modifying the tree it may leave the tree in
> > +      * an intermediate (but valid) state. As an example, adding a property
> > +      * where there is insufficient space may result in the property name
> > +      * being added to the string table even though the property itself is
> > +      * not added to the struct section.
> > +      *
> > +      * Only use this if you have a fully validated device tree with
> > +      * the latest supported version and wish to minimise code size.
> > +      */
> > +     ASSUME_PERFECT          = 0xff,
> > +
> > +     /*
> > +      * This assumes that the device tree is sane. i.e. header metadata
> > +      * and basic hierarchy are correct.
> > +      *
> > +      * It also assumes libfdt functions are called with valid parameters,
> > +      * i.e. not trigger FDT_ERR_BADOFFSET or offsets that are out of bounds.
> > +      * It disables any extensive checking of parameters and the device
> > +      * tree, making various assumptions about correctness. Normal device
> > +      * trees produced by libfdt and the compiler should be handled safely.
> > +      * Malicious device trees and complete garbage may cause libfdt to
> > +      * behave badly or crash.
> > +      *
> > +      * These checks will be sufficient if you have a valid device tree with
> > +      * no internal inconsistencies and call libfdt with correct parameters.
> > +      * With this assumption, libfdt will generally not return
> > +      * -FDT_ERR_INTERNAL, -FDT_ERR_BADLAYOUT, etc.
> > +      */
> > +     ASSUME_SANE             = 1 << 0,
>
> How tricky would it be to split this into ASSUME_VALID_DTB and
> ASSUME_VALID_PARAMETERS?  I don't know useful those are on their own,
> but it seems to me it might be easier to define the effect of this if
> split that way.

From my reading of the checks I've added, we could split out the
ASSUME_VALID_DTB assumption and that would have some checks in it
(e.g. in fdt_check_header()).

But for parameters the code doesn't currently check whether the DT or
the argument is wrong. E.g.:

const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
{
unsigned absoffset = offset + fdt_off_dt_struct(fdt);

if (!can_assume(SANE))
if ((absoffset < offset)
    || ((absoffset + len) < absoffset)
    || (absoffset + len) > fdt_totalsize(fdt))
return NULL;

Either the struct offset or @offset could be wrong here.

I think we could use ASSUME_VALID_DTB for the checks where it makes
sense. For the others we could perhaps assume that the DTB is valid
and blame any failure on parameters. So:

- anything that has no parameters in the check would use ASSUME_VALID_DTB
- anything else would use ASSUME_VALID_PARAMETERS

What do you think?

>
> > +
> > +     /*
> > +      * This disables checks for device-tree version and removes all code
> > +      * which handles older versions.
> > +      *
> > +      * Only enable this if you know you have a device tree with the latest
> > +      * version.
> > +      */
> > +     ASSUME_LATEST           = 1 << 1,
> > +
> > +     /*
> > +      * This assume that it is OK for a failed additional to the device tree
> > +      * due to lack of space or some other problem can skip any rollback
> > +      * steps (such as dropping the property name from the string table).
> > +      * This is safe to enable in most circumstances, even though it may
> > +      * leave the tree in a sub-optimal state.
> > +      */
> > +     ASSUME_NO_ROLLBACK      = 1 << 2,
> > +};
> > +
> > +/**
> > + * _can_assume() - check if a particular assumption is enabled
>
> Don't use a leading _ please, that's reserved for system libraries.
> I've moved to trailing _ for that reason.

OK.

>
> > + *
> > + * @mask: Mask to check (ASSUME_...)
> > + * @return true if that assumption is enabled, else false
> > + */
> > +static inline bool _can_assume(int mask)
> > +{
> > +     return FDT_ASSUME_MASK & mask;
> > +}
> > +
> > +/** helper macros for checking assumptions */
> > +#define can_assume(_assume)  _can_assume(ASSUME_ ## _assume)
> > +
> >  #endif /* LIBFDT_INTERNAL_H */

Regards,
Simon

Re: [PATCH v4 5/6] libfdt: Add support for disabling version checks

[Simon Glass]

Hi David,

On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Nov 12, 2019 at 06:54:21PM -0700, Simon Glass wrote:
> > Allow enabling FDT_ASSUME_LATEST to disable version checks.
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> LGTM, one minor comment below.
>
> > ---
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> >  libfdt/fdt.c    | 34 +++++++++++++++++++++-------------
> >  libfdt/fdt_ro.c | 16 ++++++++--------
> >  libfdt/fdt_rw.c |  6 +++---
> >  3 files changed, 32 insertions(+), 24 deletions(-)
> >
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index d89c0e0..78dbe58 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
> >
> >       if (fdt_magic(fdt) == FDT_MAGIC) {
> >               /* Complete tree */
> > -             if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > -                     return -FDT_ERR_BADVERSION;
> > -             if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
> > -                     return -FDT_ERR_BADVERSION;
> > +             if (!can_assume(LATEST)) {
> > +                     if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > +                             return -FDT_ERR_BADVERSION;
> > +                     if (fdt_last_comp_version(fdt) >
> > +                                     FDT_LAST_SUPPORTED_VERSION)
> > +                             return -FDT_ERR_BADVERSION;
> > +             }
> >       } else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
> >               /* Unfinished sequential-write blob */
> >               if (fdt_size_dt_struct(fdt) == 0)
> > @@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
> >
> >  size_t fdt_header_size(const void *fdt)
> >  {
> > -     return fdt_header_size_(fdt_version(fdt));
> > +     return can_assume(LATEST) ? FDT_V17_SIZE :
> > +             fdt_header_size_(fdt_version(fdt));
> >  }
> >
> >  int fdt_check_header(const void *fdt)
> > @@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
> >
> >       if (fdt_magic(fdt) != FDT_MAGIC)
> >               return -FDT_ERR_BADMAGIC;
> > -     if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > -         || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > -             return -FDT_ERR_BADVERSION;
> > -     if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > -             return -FDT_ERR_BADVERSION;
> > +     if (!can_assume(LATEST)) {
> > +             if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > +                 || (fdt_last_comp_version(fdt) >
> > +                     FDT_LAST_SUPPORTED_VERSION))
> > +                     return -FDT_ERR_BADVERSION;
> > +             if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > +                     return -FDT_ERR_BADVERSION;
> > +     }
> >       hdrsize = fdt_header_size(fdt);
> >       if (!can_assume(SANE)) {
> >
> > @@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
> >
> >       if (!can_assume(SANE)) {
> >               /* Bounds check structure block */
> > -             if (fdt_version(fdt) < 17) {
> > +             if (!can_assume(LATEST) && fdt_version(fdt) < 17) {
>
> I wonder if we should make fdt_version() return 17 unconditionally in
> the can_assume() case, which might make some of these tests redundant.

I think that would require fdt_version() to change from being a macro,
since we cannot check assumptions in libfdt.h without including
libfdt_internal.h in libfdt.h

Is that acceptable?

Regards,
Simon

Re: [PATCH v4 1/6] libfdt: De-inline fdt_header_size()

[David Gibson]

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

On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
> > > >
> > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > There does not seem to be a strong reason to inline this function.
> > > >
> > > > Well.. I think my rationale was just that this function should be a
> > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > it's not called enough for removing that to balance out the code
> > > > overhead of an extra function.  Or have you made measurements which
> > > > show I'm wrong about that?
> > >
> > > Well it isn't called in U-Boot at all, other than via
> > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > on Thumb2.
> > >
> > > But even better would be to drop fdt_check_header_(). something like:
> > >
> > > size_t fdt_header_size(uint32_t version)
> > >    {
> > >    if (fdt_chk_version()) {
> > >       if (version <= 1)
> > >          return FDT_V1_SIZE;
> > >       else if (version <= 2)
> > >          return FDT_V2_SIZE;
> > >       else if (version <= 3)
> > >          return FDT_V3_SIZE;
> > >       else if (version <= 16)
> > >          return FDT_V16_SIZE;
> > >       }
> > >
> > > return FDT_V17_SIZE;
> > > }
> > >
> > > That saves an additional 8 bytes.
> >
> > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > fdt_check_header_() to the .h file, so it can be inlined (and
> > therefore constant folded)?
> >
> > That could be a reasonable plan, though we'll need to make sure to
> > include an out-of-line version as well, for ABI compatibility.
> 
> OK that sounds like a pain with little gain. Let's leave it as it is
> then.

Actually, I suspect it's not that bad, I think "extern inline" does
what we'd need.

-- 
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 v4 5/6] libfdt: Add support for disabling version checks

[David Gibson]

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

On Fri, Jan 31, 2020 at 11:14:27AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Nov 12, 2019 at 06:54:21PM -0700, Simon Glass wrote:
> > > Allow enabling FDT_ASSUME_LATEST to disable version checks.
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >
> > LGTM, one minor comment below.
> >
> > > ---
> > >
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  libfdt/fdt.c    | 34 +++++++++++++++++++++-------------
> > >  libfdt/fdt_ro.c | 16 ++++++++--------
> > >  libfdt/fdt_rw.c |  6 +++---
> > >  3 files changed, 32 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index d89c0e0..78dbe58 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
> > >
> > >       if (fdt_magic(fdt) == FDT_MAGIC) {
> > >               /* Complete tree */
> > > -             if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > -                     return -FDT_ERR_BADVERSION;
> > > -             if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
> > > -                     return -FDT_ERR_BADVERSION;
> > > +             if (!can_assume(LATEST)) {
> > > +                     if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > +                             return -FDT_ERR_BADVERSION;
> > > +                     if (fdt_last_comp_version(fdt) >
> > > +                                     FDT_LAST_SUPPORTED_VERSION)
> > > +                             return -FDT_ERR_BADVERSION;
> > > +             }
> > >       } else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
> > >               /* Unfinished sequential-write blob */
> > >               if (fdt_size_dt_struct(fdt) == 0)
> > > @@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
> > >
> > >  size_t fdt_header_size(const void *fdt)
> > >  {
> > > -     return fdt_header_size_(fdt_version(fdt));
> > > +     return can_assume(LATEST) ? FDT_V17_SIZE :
> > > +             fdt_header_size_(fdt_version(fdt));
> > >  }
> > >
> > >  int fdt_check_header(const void *fdt)
> > > @@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
> > >
> > >       if (fdt_magic(fdt) != FDT_MAGIC)
> > >               return -FDT_ERR_BADMAGIC;
> > > -     if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > -         || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > > -             return -FDT_ERR_BADVERSION;
> > > -     if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > -             return -FDT_ERR_BADVERSION;
> > > +     if (!can_assume(LATEST)) {
> > > +             if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > +                 || (fdt_last_comp_version(fdt) >
> > > +                     FDT_LAST_SUPPORTED_VERSION))
> > > +                     return -FDT_ERR_BADVERSION;
> > > +             if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > +                     return -FDT_ERR_BADVERSION;
> > > +     }
> > >       hdrsize = fdt_header_size(fdt);
> > >       if (!can_assume(SANE)) {
> > >
> > > @@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
> > >
> > >       if (!can_assume(SANE)) {
> > >               /* Bounds check structure block */
> > > -             if (fdt_version(fdt) < 17) {
> > > +             if (!can_assume(LATEST) && fdt_version(fdt) < 17) {
> >
> > I wonder if we should make fdt_version() return 17 unconditionally in
> > the can_assume() case, which might make some of these tests redundant.
> 
> I think that would require fdt_version() to change from being a macro,
> since we cannot check assumptions in libfdt.h without including
> libfdt_internal.h in libfdt.h

Ah.. good point.

> Is that acceptable?

Making it not a macro is fine per se, but for this you'd also need to
make it out-of-line, rather than inline in libfdt.h, and I strongly
suspect that won't be worth the cost.  So, no.

-- 
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 v4 2/6] Add a way to control the level of checks in the code

[David Gibson]

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

On Fri, Jan 31, 2020 at 11:14:26AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Nov 12, 2019 at 06:54:18PM -0700, Simon Glass wrote:
> > > Add a new ASSUME_MASK option, which allows for some control over the
> > > checks used in libfdt. With all assumptions enabled, libfdt assumes that
> > > the input data and parameters are all correct and that internal errors
> > > cannot happen.
> > >
> > > By default no assumptions are made and all checks are enabled.
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > >
> > > Changes in v4:
> > > - Add a can_assume() macros and squash the inline functions into one
> > > - Drop unnecessary FDT_ prefix
> > > - Fix 'Incosistencies' typo
> > > - Merge the 'friendly' and 'sane' checks into one
> > > - Update and expand comments
> > >
> > > Changes in v3:
> > > - Expand the comments about each check option
> > > - Invert the checks to be called assumptions
> > > - Move header-version code to fdt.c
> > > - Move the comments on check options to the enum
> > > - Use hex for CHK_MASK
> > >
> > > Changes in v2:
> > > - Add an fdt_ prefix to avoid namespace conflicts
> > > - Use symbolic names for _check functions and drop leading underscores
> > >
> > >  Makefile                 |  6 ++-
> > >  libfdt/libfdt_internal.h | 88 ++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 93 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 4d88157..bdeaf93 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -16,7 +16,11 @@ EXTRAVERSION =
> > >  LOCAL_VERSION =
> > >  CONFIG_LOCALVERSION =
> > >
> > > -CPPFLAGS = -I libfdt -I .
> > > +# Control the assumptions made (e.g. risking security issues) in the code.
> > > +# See libfdt_internal.h for details
> > > +ASSUME_MASK ?= 0
> > > +
> > > +CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> > >  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
> > >       -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> > >  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
> > > diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> > > index 058c735..198480c 100644
> > > --- a/libfdt/libfdt_internal.h
> > > +++ b/libfdt/libfdt_internal.h
> > > @@ -48,4 +48,92 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
> > >
> > >  #define FDT_SW_MAGIC         (~FDT_MAGIC)
> > >
> > > +/**********************************************************************/
> > > +/* Checking controls                                                  */
> > > +/**********************************************************************/
> > > +
> > > +#ifndef FDT_ASSUME_MASK
> > > +#define FDT_ASSUME_MASK 0
> > > +#endif
> > > +
> > > +/*
> > > + * Defines assumptions which can be enabled. Each of these can be enabled
> > > + * individually. For maximum saftey, don't enable any assumptions!
> > > + *
> > > + * For minimal code size and no safety, use ASSUME_PERFECT at your own risk.
> > > + * You should have another method of validating the device tree, such as a
> > > + * signature or hash check before using libfdt.
> > > + *
> > > + * For situations where security is not a concern it may be safe to enable
> > > + * ASSUME_SANE.
> > > + */
> > > +enum {
> > > +     /*
> > > +      * This does essentially no checks. Only the latest device-tree
> > > +      * version is correctly handled. Inconsistencies or errors in the device
> > > +      * tree may cause undefined behaviour or crashes.
> >
> > Likewise for errors in parameters, yes?
> 
> Will add
> 
> >
> > > +      * If an error occurs when modifying the tree it may leave the tree in
> > > +      * an intermediate (but valid) state. As an example, adding a property
> > > +      * where there is insufficient space may result in the property name
> > > +      * being added to the string table even though the property itself is
> > > +      * not added to the struct section.
> > > +      *
> > > +      * Only use this if you have a fully validated device tree with
> > > +      * the latest supported version and wish to minimise code size.
> > > +      */
> > > +     ASSUME_PERFECT          = 0xff,
> > > +
> > > +     /*
> > > +      * This assumes that the device tree is sane. i.e. header metadata
> > > +      * and basic hierarchy are correct.
> > > +      *
> > > +      * It also assumes libfdt functions are called with valid parameters,
> > > +      * i.e. not trigger FDT_ERR_BADOFFSET or offsets that are out of bounds.
> > > +      * It disables any extensive checking of parameters and the device
> > > +      * tree, making various assumptions about correctness. Normal device
> > > +      * trees produced by libfdt and the compiler should be handled safely.
> > > +      * Malicious device trees and complete garbage may cause libfdt to
> > > +      * behave badly or crash.
> > > +      *
> > > +      * These checks will be sufficient if you have a valid device tree with
> > > +      * no internal inconsistencies and call libfdt with correct parameters.
> > > +      * With this assumption, libfdt will generally not return
> > > +      * -FDT_ERR_INTERNAL, -FDT_ERR_BADLAYOUT, etc.
> > > +      */
> > > +     ASSUME_SANE             = 1 << 0,
> >
> > How tricky would it be to split this into ASSUME_VALID_DTB and
> > ASSUME_VALID_PARAMETERS?  I don't know useful those are on their own,
> > but it seems to me it might be easier to define the effect of this if
> > split that way.
> 
> From my reading of the checks I've added, we could split out the
> ASSUME_VALID_DTB assumption and that would have some checks in it
> (e.g. in fdt_check_header()).
> 
> But for parameters the code doesn't currently check whether the DT or
> the argument is wrong. E.g.:
> 
> const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> {
> unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> 
> if (!can_assume(SANE))
> if ((absoffset < offset)
>     || ((absoffset + len) < absoffset)
>     || (absoffset + len) > fdt_totalsize(fdt))
> return NULL;
> 
> Either the struct offset or @offset could be wrong here.

Ah, I see your point.  And in some cases it's not necessarily possible
to tell - for example there are several places where it really possible to
tell - there's a bunch of cases where we can't really know whether
it's a bad offset or bad data in the tree.

> I think we could use ASSUME_VALID_DTB for the checks where it makes
> sense. For the others we could perhaps assume that the DTB is valid
> and blame any failure on parameters. So:
> 
> - anything that has no parameters in the check would use ASSUME_VALID_DTB
> - anything else would use ASSUME_VALID_PARAMETERS
> 
> What do you think?

I think that sounds workable, though I'd suggest refining it a bit.
Anything that can only be caused by bad dtb, you can elide with
ASSUME_VALID_DTB.  Anything that might be bad dtb or bad parameters
you'd need ASSUME_VALID_PARAMETERS.  Or maybe call it
ASSUME_VALID_INPUT, to imply both the dtb and all other inputs to the
library are correct?

I think that's essentially the approach we already use for the
purposes of picking what error code we return.  We should only return
BADSTRUCTURE if it's definitely bad content in the dtb.  However, in
some cases we could return BADOFFSET where the problem really likes in
the dtb, not the offset parameter.  I mean, I guess a "valid offset"
isn't even a meaningful concept if the dtb is bad.

That would mean that setting ASSUME_VALID_PARAMETERS without setting
ASSUME_VALID_DTB wouldn't make sense, which should be documented.

-- 
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 v4 1/6] libfdt: De-inline fdt_header_size()

[Simon Glass]

Hi David,

On Sat, 1 Feb 2020 at 21:38, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > > Hi David,
> > > >
> > > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > > There does not seem to be a strong reason to inline this function.
> > > > >
> > > > > Well.. I think my rationale was just that this function should be a
> > > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > > it's not called enough for removing that to balance out the code
> > > > > overhead of an extra function.  Or have you made measurements which
> > > > > show I'm wrong about that?
> > > >
> > > > Well it isn't called in U-Boot at all, other than via
> > > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > > on Thumb2.
> > > >
> > > > But even better would be to drop fdt_check_header_(). something like:
> > > >
> > > > size_t fdt_header_size(uint32_t version)
> > > >    {
> > > >    if (fdt_chk_version()) {
> > > >       if (version <= 1)
> > > >          return FDT_V1_SIZE;
> > > >       else if (version <= 2)
> > > >          return FDT_V2_SIZE;
> > > >       else if (version <= 3)
> > > >          return FDT_V3_SIZE;
> > > >       else if (version <= 16)
> > > >          return FDT_V16_SIZE;
> > > >       }
> > > >
> > > > return FDT_V17_SIZE;
> > > > }
> > > >
> > > > That saves an additional 8 bytes.
> > >
> > > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > > fdt_check_header_() to the .h file, so it can be inlined (and
> > > therefore constant folded)?
> > >
> > > That could be a reasonable plan, though we'll need to make sure to
> > > include an out-of-line version as well, for ABI compatibility.
> >
> > OK that sounds like a pain with little gain. Let's leave it as it is
> > then.
>
> Actually, I suspect it's not that bad, I think "extern inline" does
> what we'd need.

I get lots of 'multiple definition' errors when I add 'extern inline'
to the header file for fdt_header_size().

From here:

https://stackoverflow.com/questions/216510/what-does-extern-inline-do

   extern inline: like GNU89 "inline": externally visible code is
emitted, so at most one translation unit can use this.

Can you explain a bit more about what you are thinking here? I am a
bit lost. Remember that in my next patch I have to check the
assumptions in libfdt_internl.h

Regards,
Simon

Re: [PATCH v4 1/6] libfdt: De-inline fdt_header_size()

[David Gibson]

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

On Wed, Feb 05, 2020 at 01:57:01PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Sat, 1 Feb 2020 at 21:38, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
> > > >
> > > > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > > > Hi David,
> > > > >
> > > > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnQMXKaNCjofrA@public.gmane.org.id.au> wrote:
> > > > > >
> > > > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > > > There does not seem to be a strong reason to inline this function.
> > > > > >
> > > > > > Well.. I think my rationale was just that this function should be a
> > > > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > > > it's not called enough for removing that to balance out the code
> > > > > > overhead of an extra function.  Or have you made measurements which
> > > > > > show I'm wrong about that?
> > > > >
> > > > > Well it isn't called in U-Boot at all, other than via
> > > > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > > > on Thumb2.
> > > > >
> > > > > But even better would be to drop fdt_check_header_(). something like:
> > > > >
> > > > > size_t fdt_header_size(uint32_t version)
> > > > >    {
> > > > >    if (fdt_chk_version()) {
> > > > >       if (version <= 1)
> > > > >          return FDT_V1_SIZE;
> > > > >       else if (version <= 2)
> > > > >          return FDT_V2_SIZE;
> > > > >       else if (version <= 3)
> > > > >          return FDT_V3_SIZE;
> > > > >       else if (version <= 16)
> > > > >          return FDT_V16_SIZE;
> > > > >       }
> > > > >
> > > > > return FDT_V17_SIZE;
> > > > > }
> > > > >
> > > > > That saves an additional 8 bytes.
> > > >
> > > > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > > > fdt_check_header_() to the .h file, so it can be inlined (and
> > > > therefore constant folded)?
> > > >
> > > > That could be a reasonable plan, though we'll need to make sure to
> > > > include an out-of-line version as well, for ABI compatibility.
> > >
> > > OK that sounds like a pain with little gain. Let's leave it as it is
> > > then.
> >
> > Actually, I suspect it's not that bad, I think "extern inline" does
> > what we'd need.
> 
> I get lots of 'multiple definition' errors when I add 'extern inline'
> to the header file for fdt_header_size().
> 
> >From here:
> 
> https://stackoverflow.com/questions/216510/what-does-extern-inline-do
> 
>    extern inline: like GNU89 "inline": externally visible code is
> emitted, so at most one translation unit can use this.
> 
> Can you explain a bit more about what you are thinking here? I am a
> bit lost. Remember that in my next patch I have to check the
> assumptions in libfdt_internl.h

Ah, I guess I was wrong.  Ok this will need a closer look.

-- 
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 v4 1/6] libfdt: De-inline fdt_header_size()

[Simon Glass]

Hi David,

On Wed, 5 Feb 2020 at 21:53, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Wed, Feb 05, 2020 at 01:57:01PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Sat, 1 Feb 2020 at 21:38, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> > > > Hi David,
> > > >
> > > > On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > > > > There does not seem to be a strong reason to inline this function.
> > > > > > >
> > > > > > > Well.. I think my rationale was just that this function should be a
> > > > > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > > > > it's not called enough for removing that to balance out the code
> > > > > > > overhead of an extra function.  Or have you made measurements which
> > > > > > > show I'm wrong about that?
> > > > > >
> > > > > > Well it isn't called in U-Boot at all, other than via
> > > > > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > > > > on Thumb2.
> > > > > >
> > > > > > But even better would be to drop fdt_check_header_(). something like:
> > > > > >
> > > > > > size_t fdt_header_size(uint32_t version)
> > > > > >    {
> > > > > >    if (fdt_chk_version()) {
> > > > > >       if (version <= 1)
> > > > > >          return FDT_V1_SIZE;
> > > > > >       else if (version <= 2)
> > > > > >          return FDT_V2_SIZE;
> > > > > >       else if (version <= 3)
> > > > > >          return FDT_V3_SIZE;
> > > > > >       else if (version <= 16)
> > > > > >          return FDT_V16_SIZE;
> > > > > >       }
> > > > > >
> > > > > > return FDT_V17_SIZE;
> > > > > > }
> > > > > >
> > > > > > That saves an additional 8 bytes.
> > > > >
> > > > > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > > > > fdt_check_header_() to the .h file, so it can be inlined (and
> > > > > therefore constant folded)?
> > > > >
> > > > > That could be a reasonable plan, though we'll need to make sure to
> > > > > include an out-of-line version as well, for ABI compatibility.
> > > >
> > > > OK that sounds like a pain with little gain. Let's leave it as it is
> > > > then.
> > >
> > > Actually, I suspect it's not that bad, I think "extern inline" does
> > > what we'd need.
> >
> > I get lots of 'multiple definition' errors when I add 'extern inline'
> > to the header file for fdt_header_size().
> >
> > >From here:
> >
> > https://stackoverflow.com/questions/216510/what-does-extern-inline-do
> >
> >    extern inline: like GNU89 "inline": externally visible code is
> > emitted, so at most one translation unit can use this.
> >
> > Can you explain a bit more about what you are thinking here? I am a
> > bit lost. Remember that in my next patch I have to check the
> > assumptions in libfdt_internl.h
>
> Ah, I guess I was wrong.  Ok this will need a closer look.

So how about we go with this patch as it?

Regards,
Simon

Re: [PATCH v4 1/6] libfdt: De-inline fdt_header_size()

[David Gibson]

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

On Wed, Feb 05, 2020 at 10:03:37PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Wed, 5 Feb 2020 at 21:53, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Wed, Feb 05, 2020 at 01:57:01PM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Sat, 1 Feb 2020 at 21:38, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> > > >
> > > > On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> > > > > Hi David,
> > > > >
> > > > > On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnQMXKaNCjofrA@public.gmane.org.id.au> wrote:
> > > > > >
> > > > > > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > > > > > Hi David,
> > > > > > >
> > > > > > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnR4Cd2VRjVQmg@public.gmane.orgbear.id.au> wrote:
> > > > > > > >
> > > > > > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > > > > > There does not seem to be a strong reason to inline this function.
> > > > > > > >
> > > > > > > > Well.. I think my rationale was just that this function should be a
> > > > > > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > > > > > it's not called enough for removing that to balance out the code
> > > > > > > > overhead of an extra function.  Or have you made measurements which
> > > > > > > > show I'm wrong about that?
> > > > > > >
> > > > > > > Well it isn't called in U-Boot at all, other than via
> > > > > > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > > > > > on Thumb2.
> > > > > > >
> > > > > > > But even better would be to drop fdt_check_header_(). something like:
> > > > > > >
> > > > > > > size_t fdt_header_size(uint32_t version)
> > > > > > >    {
> > > > > > >    if (fdt_chk_version()) {
> > > > > > >       if (version <= 1)
> > > > > > >          return FDT_V1_SIZE;
> > > > > > >       else if (version <= 2)
> > > > > > >          return FDT_V2_SIZE;
> > > > > > >       else if (version <= 3)
> > > > > > >          return FDT_V3_SIZE;
> > > > > > >       else if (version <= 16)
> > > > > > >          return FDT_V16_SIZE;
> > > > > > >       }
> > > > > > >
> > > > > > > return FDT_V17_SIZE;
> > > > > > > }
> > > > > > >
> > > > > > > That saves an additional 8 bytes.
> > > > > >
> > > > > > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > > > > > fdt_check_header_() to the .h file, so it can be inlined (and
> > > > > > therefore constant folded)?
> > > > > >
> > > > > > That could be a reasonable plan, though we'll need to make sure to
> > > > > > include an out-of-line version as well, for ABI compatibility.
> > > > >
> > > > > OK that sounds like a pain with little gain. Let's leave it as it is
> > > > > then.
> > > >
> > > > Actually, I suspect it's not that bad, I think "extern inline" does
> > > > what we'd need.
> > >
> > > I get lots of 'multiple definition' errors when I add 'extern inline'
> > > to the header file for fdt_header_size().
> > >
> > > >From here:
> > >
> > > https://stackoverflow.com/questions/216510/what-does-extern-inline-do
> > >
> > >    extern inline: like GNU89 "inline": externally visible code is
> > > emitted, so at most one translation unit can use this.
> > >
> > > Can you explain a bit more about what you are thinking here? I am a
> > > bit lost. Remember that in my next patch I have to check the
> > > assumptions in libfdt_internl.h
> >
> > Ah, I guess I was wrong.  Ok this will need a closer look.
> 
> So how about we go with this patch as it?

Yeah, go with whatever is best for you in the next spin, and we'll see
how it looks.

-- 
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 v4 1/6] libfdt: De-inline fdt_header_size()

[Simon Glass]

Hi David,

On Wed, 5 Feb 2020 at 22:32, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Wed, Feb 05, 2020 at 10:03:37PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Wed, 5 Feb 2020 at 21:53, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 01:57:01PM -0700, Simon Glass wrote:
> > > > Hi David,
> > > >
> > > > On Sat, 1 Feb 2020 at 21:38, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Fri, Jan 31, 2020 at 11:14:25AM -0700, Simon Glass wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > On Tue, 28 Jan 2020 at 01:37, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> > > > > > > > Hi David,
> > > > > > > >
> > > > > > > > On Tue, 19 Nov 2019 at 21:01, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > > > > > > > > > There does not seem to be a strong reason to inline this function.
> > > > > > > > >
> > > > > > > > > Well.. I think my rationale was just that this function should be a
> > > > > > > > > single load, followed by the call to fdt_header_size_().  I suspect
> > > > > > > > > it's not called enough for removing that to balance out the code
> > > > > > > > > overhead of an extra function.  Or have you made measurements which
> > > > > > > > > show I'm wrong about that?
> > > > > > > >
> > > > > > > > Well it isn't called in U-Boot at all, other than via
> > > > > > > > fdt_check_header(). The inlined version saves 16 bytes of code space
> > > > > > > > on Thumb2.
> > > > > > > >
> > > > > > > > But even better would be to drop fdt_check_header_(). something like:
> > > > > > > >
> > > > > > > > size_t fdt_header_size(uint32_t version)
> > > > > > > >    {
> > > > > > > >    if (fdt_chk_version()) {
> > > > > > > >       if (version <= 1)
> > > > > > > >          return FDT_V1_SIZE;
> > > > > > > >       else if (version <= 2)
> > > > > > > >          return FDT_V2_SIZE;
> > > > > > > >       else if (version <= 3)
> > > > > > > >          return FDT_V3_SIZE;
> > > > > > > >       else if (version <= 16)
> > > > > > > >          return FDT_V16_SIZE;
> > > > > > > >       }
> > > > > > > >
> > > > > > > > return FDT_V17_SIZE;
> > > > > > > > }
> > > > > > > >
> > > > > > > > That saves an additional 8 bytes.
> > > > > > >
> > > > > > > Uh.. I'm a little lost here.  Is what you're suggesting moving all of
> > > > > > > fdt_check_header_() to the .h file, so it can be inlined (and
> > > > > > > therefore constant folded)?
> > > > > > >
> > > > > > > That could be a reasonable plan, though we'll need to make sure to
> > > > > > > include an out-of-line version as well, for ABI compatibility.
> > > > > >
> > > > > > OK that sounds like a pain with little gain. Let's leave it as it is
> > > > > > then.
> > > > >
> > > > > Actually, I suspect it's not that bad, I think "extern inline" does
> > > > > what we'd need.
> > > >
> > > > I get lots of 'multiple definition' errors when I add 'extern inline'
> > > > to the header file for fdt_header_size().
> > > >
> > > > >From here:
> > > >
> > > > https://stackoverflow.com/questions/216510/what-does-extern-inline-do
> > > >
> > > >    extern inline: like GNU89 "inline": externally visible code is
> > > > emitted, so at most one translation unit can use this.
> > > >
> > > > Can you explain a bit more about what you are thinking here? I am a
> > > > bit lost. Remember that in my next patch I have to check the
> > > > assumptions in libfdt_internl.h
> > >
> > > Ah, I guess I was wrong.  Ok this will need a closer look.
> >
> > So how about we go with this patch as it?
>
> Yeah, go with whatever is best for you in the next spin, and we'll see
> how it looks.

OK I'll send what I have since it is definitely closer to what you suggested.

Regards,
Simon