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.
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?
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?
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.
Regards,
Simon
