On Thu, Aug 01, 2019 at 01:13:58PM -0600, Simon Glass wrote:
> Add a new CHECK_MASK option, which allows for some control over the
> checks used in libfdt. With no checks enabled, libfdt assumes that the
> input data and parameters are all correct and that internal errors cannot
> happen.
>
> By default all checks are enabled.
>
> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
Sorry I've taken so long to look at this. I think I'm pretty much
convinced by this in outline, just some details to polish.
> ---
>
> 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.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index e955242..9aab665 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -16,7 +16,11 @@ EXTRAVERSION =
> LOCAL_VERSION =
> CONFIG_LOCALVERSION =
>
> -CPPFLAGS = -I libfdt -I .
> +# Control the checks (e.g. to avoid security issues) to include in the code
> +# See libfdt_internal.h for details
> +CHK_MASK ?= 255
Hex for the masks, please.
Also, you mention libfdt_internal.h here, but then change libfdt.h
below.
I think libfdt_internal.h is actually the right choice, see further
comments below.
> +
> +CPPFLAGS = -I libfdt -I . -DFDT_CHK_MASK=$(CHK_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.h b/libfdt/libfdt.h
> index 8037f39..5562184 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -172,6 +172,53 @@ static inline void fdt64_st(void *property, uint64_t value)
> bp[7] = value & 0xff;
> }
>
> +/**********************************************************************/
> +/* Checking controls */
> +/**********************************************************************/
> +
> +#ifndef FDT_CHK_MASK
> +#define FDT_CHK_MASK 255
> +#endif
> +
> +/* Defines the available checks, each of which can be enabled individually */
> +enum {
> + FDT_MASK_CHK_NONE = 0,
> + FDT_MASK_CHK_BASIC = 1 << 0,
> + FDT_MASK_CHK_VERSION = 1 << 1,
> + FDT_MASK_CHK_EXTRA = 1 << 2,
I'd prefer to see the comments about what each check covers here,
rather than on the wrapping functions below, so that they're in one
place.
> +};
> +
> +/**
> + * fdt_chk_basic() - see if basic checking of params and DT data is enabled
> + *
> + * This level assumes that the device tree is sane (header metadata and basic
> + * hierarchy are correct). Old device-tree versions are handled
> correctly.
This looks like it needs a bit of an update for the mask based system.
I'd like to see these descriptions expanded a bit too, in particular
each one should make clear what are the prerequisites on the dtb to
work with this option. So, for basic you need a valid tree, for
version you need a v17 tree.
A general comment in this section should emphasise that turning off
checks will *messily* fail if your tree doesn't meet the requirements.
> + */
> +static inline bool fdt_chk_basic(void)
> +{
> + return FDT_CHK_MASK & FDT_MASK_CHK_BASIC;
> +}
> +
> +/**
> + * fdt_chk_version() - see if we need to handle old versions of the DT
> + *
> + */
> +static inline bool fdt_chk_version(void)
> +{
> + return FDT_CHK_MASK & FDT_MASK_CHK_VERSION;
> +}
> +
> +/**
> + * fdt_chk_extra() - see if extra checking is enabled
> + *
> + * This check enables more extensive checking of parameters and the device tree,
> + * making few assumptions about correctness.
> + */
> +static inline bool fdt_chk_extra(void)
> +{
> + return FDT_CHK_MASK & FDT_MASK_CHK_EXTRA;
> +}
> +
> /**********************************************************************/
> /* Traversal functions */
> /**********************************************************************/
> @@ -269,7 +316,8 @@ fdt_set_hdr_(size_dt_struct);
> 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));
> + return fdt_chk_version() ? fdt_header_size_(fdt_version(fdt)) :
> + FDT_V17_SIZE;
So, AFAICT, handling this is the one reason you need the stuff in
libfdt.h rather than libfdt_internal.h. But even here, the results
could be kinda weird - if a user directly calls fdt_header_size() the
checking-ness will depend on FDT_CHK_MASK in the caller's build rather
than libfdt's build.
I think a better approach is to de-inline fdt_header_size(), at least
for external callers.
--
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
