Hi David, On Mon, 22 Jul 2019 at 01:25, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > 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. OK > > > + > > +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. I do prefer inlines since it makes the code a bit shorter. But I could open-code the mask if you prefer. > > 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. Indeed. I've changed to a mask in v2 to see if you like that better. Regards, Simon
