Hi David,
On Tue, 29 Oct 2019 at 02:00, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 24, 2019 at 09:29:21PM -0600, 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@xxxxxxxxxxxx>
>
> Overall scheme looks good. A few minor points noted below.
>
> > ---
> >
> > 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/fdt.c | 3 +-
> > libfdt/libfdt_internal.h | 83 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 90 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index e955242..a5e6984 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/fdt.c b/libfdt/fdt.c
> > index 3e37a4b..b706e42 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -72,7 +72,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 fdt_chk_version() ? fdt_header_size_(fdt_version(fdt)) :
> > + FDT_V17_SIZE;
> > }
> >
> > int fdt_check_header(const void *fdt)
> > diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> > index 741eeb3..33b6cd3 100644
> > --- a/libfdt/libfdt_internal.h
> > +++ b/libfdt/libfdt_internal.h
> > @@ -48,4 +48,87 @@ 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 FDT_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
> > + * FDT_ASSUME_FRIENDLY.
> > + */
> > +enum {
> > + /*
> > + * This does essentially no checks. Only the latest device-tree
> > + * version is correctly handled. Incosistencies or errors in the device
>
> Typo ^
OK
>
> > + * tree may cause undefined behaviour or crashes.
> > + *
> > + * 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.
> > + */
> > + FDT_ASSUME_PERFECT = 0xff,
> > +
> > + /*
> > + * This assumes that the device tree is sane. i.e. header metadata
> > + * and basic hierarchy are correct.
> > + *
> > + * These checks will be sufficient if you have a valid device tree with
> > + * no internal inconsistencies. With this assumption, libfdt will
> > + * generally not return -FDT_ERR_INTERNAL, -FDT_ERR_BADLAYOUT, etc.
> > + */
> > + FDT_ASSUME_SANE = 1 << 0,
> > +
> > + /*
> > + * 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.
> > + */
> > + FDT_ASSUME_LATEST = 1 << 1,
> > +
> > + /*
> > + * This 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.
> > + */
> > + FDT_ASSUME_FRIENDLY = 1 << 2,
>
> Hrm. I'd kind of prefer to be explicit about exactly what's assumed
> for each flag, if we possibly can be.
I was trying to be explicit. Can you give more info on what you are
looking for here?
>
> > +};
> > +
> > +/** fdt_chk_basic() - see if basic checking of params and DT data is enabled */
> > +static inline bool fdt_chk_basic(void)
>
> I think it might be cleaner to have a single inline wrapper that takes
> one of the flags as arguments.
OK, like fdt_chk(FDT_ASSUME_LATEST) ?
Regards,
Simon
