On Tue, Feb 11, 2020 at 06:23:46PM -0700, Simon Glass wrote:
> Hi David,
>
> On Tue, 11 Feb 2020 at 18:12, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Feb 11, 2020 at 01:08:42PM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Mon, 10 Feb 2020 at 18:12, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> > > > > Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> > > > > tree and the parameters to libfdt. This assumption covers that cases where
> > > > > the problem could be with either.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > Changes in v5:
> > > > > - Include just VALID_INPUT checks in this patch
> > > > >
> > > > > Changes in v4: None
> > > > > Changes in v3: None
> > > > > Changes in v2: None
> > > > >
> > > > > libfdt/fdt.c | 12 +++++----
> > > > > libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
> > > > > 2 files changed, 54 insertions(+), 29 deletions(-)
> > > > >
> > > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > > index 03f2b7d..e2c1da0 100644
> > > > > --- a/libfdt/fdt.c
> > > > > +++ b/libfdt/fdt.c
> > > > > @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> > > > > {
> > > > > unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> > > > >
> > > > > - if ((absoffset < offset)
> > > > > - || ((absoffset + len) < absoffset)
> > > > > - || (absoffset + len) > fdt_totalsize(fdt))
> > > > > - return NULL;
> > > > > + if (!can_assume(VALID_INPUT))
> > > > > + if ((absoffset < offset)
> > > > > + || ((absoffset + len) < absoffset)
> > > > > + || (absoffset + len) > fdt_totalsize(fdt))
> > > > > + return NULL;
> > > > >
> > > > > if (fdt_version(fdt) >= 0x11)
> > > > > if (((offset + len) < offset)
> > > > > @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > > return FDT_END;
> > > > > }
> > > > >
> > > > > - if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > > > + if (!can_assume(VALID_INPUT) &&
> > > > > + !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > > > return FDT_END; /* premature end */
> > > > >
> > > > > *nextoffset = FDT_TAGALIGN(offset);
> > > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > > index b41083f..07c13c9 100644
> > > > > --- a/libfdt/fdt_ro.c
> > > > > +++ b/libfdt/fdt_ro.c
> > > > > @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
> > > > > int olen;
> > > > > const char *p = fdt_get_name(fdt, offset, &olen);
> > > > >
> > > > > - if (!p || olen < len)
> > > > > + if (!p || (!can_assume(VALID_INPUT) && olen < len))
> > > >
> > > > Oof, this one's subtle. We certainly *can* have olen < len even in
> > > > perfectly valid cases. However, if we're assuming validly \0
> > > > terminated strings in the strings block *and* no \0s in s (which you
> > > > can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
> > > > up that case.
> > > >
> > > > If we also assume memcmp() is the obvious byte-by-byte implementation
> > > > then it will stop before accessing beyond the end of the strings block
> > > > string. But... I don't think that's necessarily the case for all C
> > > > libraries / runtimes. So, if this is close to the end of the strings
> > > > block, the memcmp() could access beyond the dtb buffer, which is a no
> > > > no.
> > > >
> > > > Now, I guess we could have an assume(DUMB_MEMCMP) and/or
> > > > assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
> > > > getting really esoteric now.
> > > >
> > > > Is avoiding this one comparison worth it?
> > >
> > > For further context see this commit:
> > >
> > > f1879e1 Add limited read-only support for older (V2 and V3) device
> > > tree to libfdt.
> > >
> > > Before that we assumed that it was safe to do the memcmp(), so I
> > > figured we could revert the behaviour with this assumption.
> >
> > We did, but I'm pretty sure that assumption was wrong. I think we've
> > had Coverity complain about a similar construct at some point.
>
> OK.
>
> >
> > > Another point is that fdt_subnode_offset_namelen() should probably not
> > > allow namelen to be less than strlen(name). Should we add a comment
> > > and check for that?
> >
> > Absolutely not. namelen is allowed to be less that strlen(name) and
> > expected to in many common cases. In general the _namelen() variants
> > aren't (primarily) about an optimization to avoid a strlen() call.
> >
> > Rather, they are to allow callers to use these interfaces with a piece
> > of a larger \0 terminated string without having to either mangle their
> > longer string in place, or copy sections out.
> >
> > For example fdt_path_offset() will use namelen < strlen all the time,
> > because it repeatedly calls subnode_offset_namelen() on individual
> > path components without copying them out of the path. Copying them
> > out would a) be expensive and b) without an allocator would require an
> > arbitrary length limit.
>
> OK I had it around the wrong way. So what does the comment 'short match' mean?
I guess it means "the string in the strings block is shorter than the
given string, and therefore doesn't match". We should probably just
drop the comment, it's not very illuminating.
> > > Also I don't think I fully understand fdt_nodename_eq_(). It doesn't
> > > have a function comment so it's not really clear what it is supposed
> > > to do. But if I call it with:
> > >
> > > fdt_nodename_eq_(fdt, offset, s="ernie", len=5)
> > >
> > > and say that fdt_get_name() returns "fred" with olen=4.
> > >
> > > Now olen < len is true, so this function will return 0, indicating a
> > > match. But there is no match. What am I missing?
> >
> > 1 is a match, not 0. Think of it as returning a boolean (that's why
> > the name is 'eq', not 'cmp').
>
> OK. I wonder if we could use stdbool?
Yeah, maybe. I try to be conservative with what C features I use in
libfdt, for the a sake bootloader environments built with weird old
toolchains. I don't really have a sense of how widely/long stdbool
has been implemented.
> > > Anyway I agree it doesn't seem worth it, but I'd like to get some
> > > comments into these functions.
> > >
> > > >
> > > > > /* short match */
> > > > > return 0;
> > > > >
> > >
> > > [..]
> > >
> > > > > @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > > > if (!can_assume(VALID_DTB)) {
> > > > > if ((err = fdt_ro_probe_(fdt)) < 0)
> > > > > goto fail;
> > > > > - if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > - goto fail;
> > > > > + if (can_assume(VALID_INPUT) &&
> > > >
> > > > That should be !can_assume, no?
> > >
> > > Yes, although I've dropped it in v6 since the check is now in
> > > fdt_check_node_offset_().
> > > >
> > > > > + (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > + goto fail;
> > > > > }
> > > > >
> > > > > nameptr = nh->name;
> > > > > @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
> > > > > int err;
> > > > > const struct fdt_property *prop;
> > > > >
> > > > > - if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > > > + if (!can_assume(VALID_INPUT) &&
> > > > > + (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > > > if (lenp)
> > > > > *lenp = err;
> > > > > return NULL;
> > > > > @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
> > > > > (offset = fdt_next_property_offset(fdt, offset))) {
> > > > > const struct fdt_property *prop;
> > > > >
> > > > > - if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> > > > > + prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> > > > > + if (!can_assume(VALID_INPUT) && !prop) {
> > > > > offset = -FDT_ERR_INTERNAL;
> > > >
> > > > So, arguably you could put this one under a weaker assumption flag.
> > > > Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
> > > > input - they're basically assert()s, except I didn't want to rely on
> > > > the runtime things that assert() needs.
> > >
> > > Yes I see. The fdt_first/next_property_offset() functions should never
> > > return anything invalid.
> >
> > Right. Actually, I guess this could happen if something is
> > concurrently modifying the fdt blob, but really if that's happening
> > all bets are off - there are some limits to how safe I care to be :).
>
> Indeed.
>
> Regards,
> Simon
>
--
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
