On Tue, Nov 05, 2019 at 05:46:12AM -0700, Simon Glass wrote:
> 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?
Hm, well, I forget exactly what this parameter did. But maybe
something like:
* Assumes user never gives parameter values which (would trigger
FDT_ERR_BADOFFSET | offsets that are out of bounds)
Or something along those lines.
> > > +};
> > > +
> > > +/** 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) ?
Something like that, yes. Note that because these are inlines and in
fdt_internal.h, rather than the published libfdt.h, you don't have to
have the fdt_() prefix for namespacing. So you could do something
like
can_assume(LATEST)
with a little cpp magic.
>
> Regards,
> Simon
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
