On Sat, Jul 20, 2019 at 05:31:06PM -0600, Simon Glass wrote:
> Add a new CHECK_LEVEL option, which allows for some control over the
> checks used in libfdt. At the minimum level, libfdt assumes that the input
> data and parameters are all correct and that internal errors cannot
> happen.
>
> By default all checks are enabled.
So, I find making the checks conditional pretty ugly, but I
reluctantly accept the need for it.
There are a bunch of details I'd like to see polished here, though.
>
> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> ---
>
> Makefile | 10 +++++++++-
> libfdt/libfdt.h | 41 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 98f74b1..1ed7115 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -16,7 +16,15 @@ EXTRAVERSION =
> LOCAL_VERSION =
> CONFIG_LOCALVERSION =
>
> -CPPFLAGS = -I libfdt -I .
> +# Set the level of checking (e.g. to avoid security issues) to include in the
> +# code:
> +# 0 - no checking (minimal code size)
> +# 1 - very basic checking, assuming DT is sane but may be an old version
> +# 2 - most checking, will catch most DT and API parameter inconsistencies
> +# 3 - all possible checks with no regard to performance or code size
> +CHECK_LEVEL ?= 3
I'd prefer symbolic names to bare numbers. As well as being more
readable, it means we can insert and renumber one if we think of a
level that it's not yet obvious we need.
We might have to do a hack and use a bare number in the Makefile, but
we can at least use real names in the actual code.
> +
> +CPPFLAGS = -I libfdt -I . -DCHECK_LEVEL=$(CHECK_LEVEL)
> WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
> -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
> CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS)
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 8037f39..3277468 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -172,6 +172,47 @@ static inline void fdt64_st(void *property, uint64_t value)
> bp[7] = value & 0xff;
> }
>
> +/**********************************************************************/
> +/* Checking controls */
> +/**********************************************************************/
> +
> +#ifndef CHECK_LEVEL
This is in an exposed header. If possible I'd prefer to put it in
libfdt_internal.h. If not (e.g. because inlines in this file need
it), then we need to namespace this macro: e.g. FDT_CHECK_LEVEL.
> +#define CHECK_LEVEL 3
> +#endif
> +
> +/**
> + * _check1() - see if basic checking of parameters 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.
> + */
> +static inline bool _check1(void)
There are several problems with the naming here.
First, I'm not sure that these inlines really give us anything over
just open-coding a test on CHECK_LEVEL.
Second, a leading _ is reserved for the base C library, so we
shouldn't use it (we used to have a bunch of this, but I've been
changing them over to trailing underscores or other approaches).
Third, again this is in an exposed header. It should either move to
libfdt_internal.h, or be namespaced.
Finally the comment is kind of misleading - it describes what happens
with CHECK_LEVEL == 1, but the function tests for CHEKC_LEVEL >= 1.
> +{
> + return CHECK_LEVEL >= 1;
> +}
> +
> +/**
> + * _check2() - see if normal checking of parameters and DT data is enabled
> + *
> + * This level enables extensive checking of parameters and the device tree,
> + * making few assumptions about correctness.
> + */
> +static inline bool _check2(void)
> +{
> + return CHECK_LEVEL >= 2;
> +}
> +
> +/**
> + * _check3() - see if full checking is enabled
> + *
> + * This level enables all possible checks with no regard to performance or code
> + * size.
> + */
> +static inline bool _check3(void)
> +{
> + return CHECK_LEVEL >= 3;
> +}
> +
> /**********************************************************************/
> /* Traversal functions */
> /**********************************************************************/
--
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
