On Tue, Sep 18, 2018 at 9:38 PM David Gibson
<david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Sep 18, 2018 at 04:46:38PM -0700, Rob Herring wrote:
> > Since commit 7975f6422260 ("Fix widespread incorrect use of strneq(),
> > replace with new strprefixeq()") simple-bus checks have been silently
> > skipped. The problem was 'end - str' is one more than the string length
> > and the strnlen in strprefixeq fails. This can't be fixed simply by
> > subtracting one as it is possible to have multiple '\0' at the end of
> > the property. So first check the property is null terminated and then
> > use streq() for comparisons.
> >
> > Add some tests so the problem doesn't happen again.
> >
> > Fixes: 7975f6422260 ("Fix widespread incorrect use of strneq(), replace with new strprefixeq()")
> > Reported-by: Kumar Gala <kumar.gala@xxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>
> So, the existing version was definitely wrong. The second parameter
> to strprefixeq needs to be the exact length of the prefix to compare,
> and it's clearly not here. But your proposed version isn't quite
> right either.
>
> > ---
> > checks.c | 8 ++++++--
> > tests/run_tests.sh | 4 ++++
> > tests/unit-addr-simple-bus-compatible.dts | 18 ++++++++++++++++++
> > tests/unit-addr-simple-bus-reg-mismatch.dts | 18 ++++++++++++++++++
> > 4 files changed, 46 insertions(+), 2 deletions(-)
> > create mode 100644 tests/unit-addr-simple-bus-compatible.dts
> > create mode 100644 tests/unit-addr-simple-bus-reg-mismatch.dts
> >
> > diff --git a/checks.c b/checks.c
> > index 9c9b0c328af6..1ccbfa60c839 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -908,9 +908,13 @@ static bool node_is_compatible(struct node *node, const char *compat)
> > if (!prop)
> > return false;
> >
> > - for (str = prop->val.val, end = str + prop->val.len; str < end;
> > + str = prop->val.val;
> > + if (strnlen(str, prop->val.len) == prop->val.len)
> > + return false;
>
> This will catch the case where the property has no \0s at all, but not
> a case like say...
> compatible = "foo", /bits/ 8 <'s' 'i' 'm'>;
That's just crap. We do also validate already that compatible is a
string list. Not sure offhand if that's a dependency for this.
Maybe I should just check that the last byte is null.
> > +
> > + for (end = str + prop->val.len; str < end;
> > str += strnlen(str, end - str) + 1) {
> > - if (strprefixeq(str, end - str, compat))
> > + if (streq(str, compat))
>
> ...which will lead to this streq() running over the end of the property.
>
> I think you instead will need something like this:
> len = strlen(compat);
> /* ... */
> if ((end - str) > len && (memcmp(str, compat, len) == 0))
> /* ... */
That would match if say we have "simple-bus-foo" and compat is
"simple-bus". The memcmp would need to be len + 1, but then that is
just back to strcmp aka streq.
Rob
