netdev - Re: [PATCH iproute2 v2 1/2] bpf: Fix segfault when custom pinning is used

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Apr 2020 16:43:19 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Andrea Claudi <aclaudi@...hat.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-netdev <netdev@...r.kernel.org>,
        David Ahern <dsahern@...il.com>, daniel@...earbox.net,
        Jamal Hadi Salim <hadi@...atatu.com>
Subject: Re: [PATCH iproute2 v2 1/2] bpf: Fix segfault when custom pinning is
 used

Andrea Claudi wrote on Wed, Apr 22, 2020:
> > -       ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> > +       ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> 
> Shouldn't we check for the last argument length before? We should have
> enough space for "/../" and the terminator, so we need the last
> argument length to be less than PATH_MAX-5, right?

snprintf will return the length that would be written if there had been
room so most codes just check the return value instead (something like
if (ret >= sizeof(tmp)) error)

OTOH this will actually be caught by the later strcat guard, because rem
will always contain at least on / the while will always be entered and
strlen(tmp) + (>=0) + 2 will always be > PATH_MAX, so this function will
error out.
I'll admit it's not clear, though :)

I'm actually not sure snprintf can return < 0... wide character
formatting functions can but basic ones not really, there's hardly any
snprintf return checking in iproute2 code...


Anyway, with all that said this patch currently technically works for
me, despite being not so clear, so can add my reviewed-by on whatever
final version you take (check < 0 or >= PATH_MAX or none or both), if
you'd like :)

-- 
Dominique