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 ^
> + * 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.
> +};
> +
> +/** 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.
> +{
> + return !(FDT_ASSUME_MASK & FDT_ASSUME_SANE);
> +}
> +
> +/** fdt_chk_version() - see if we need to handle old versions of the DT */
> +static inline bool fdt_chk_version(void)
> +{
> + return !(FDT_ASSUME_MASK & FDT_ASSUME_LATEST);
> +}
> +
> +/** fdt_chk_extra() - see if extra checking is enabled */
> +static inline bool fdt_chk_extra(void)
> +{
> + return !(FDT_ASSUME_MASK & FDT_ASSUME_FRIENDLY);
> +}
> +
> #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:
signature.asc
Description: PGP signature
