netdev - Re: [PATCH iproute2 1/1] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <478bfaf8-6418-2d37-cae6-88b113d686b0@mojatatu.com>
Date:   Tue, 21 Apr 2020 15:58:36 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Andrea Claudi <aclaudi@...hat.com>
Cc:     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 1/1] bpf: Fix segfault when custom pinning is
 used

Hi Andrea,

On 2020-04-21 3:38 p.m., Andrea Claudi wrote:
[..]
> 
> Hi Jamal,
> Are you sure this fixes your issue?

Yes.

> I think asprintf could segfault
> only if ctx is null, but this case is not addressed in your patch.

The issue is tmp(after it is created by asprintf) gets trampled.
This:
ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
allocates enough space for tmp.
But then later:
strcat(tmp, sub);
strcat(tmp...);
creates a buffer overrun.

It is easy to overlook things like these when making a large
semantically-equivalent change - so i would suggest to also
review the general patch that went from sprintf->asprintf.

> I remember that Stephen asked me to use asprintf to avoid allocating
> huge buffers on stack; anyway I've no objection to sprintf, if needed.

Generally that is good practise. And even for this case
you could probably find a simpler way to solve it with asprintf
or realloc trickery. I am not sure it is worth the bother - 4K on
the stack in user space is not a big deal really.

cheers,
jamal