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?
>
> > 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?
>
> > 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
