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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > >
> > > 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:
signature.asc
Description: PGP signature
