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]

Date:   Wed, 22 Apr 2020 00:10:23 +0200
From:   Andrea Claudi <aclaudi@...hat.com>
To:     Jamal Hadi Salim <jhs@...atatu.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

On Tue, Apr 21, 2020 at 9:58 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> 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.
>

Oh, now I see. Thanks for pointing it out and making it clear to me.
I agree with you, this needs to be carefully reviewed to ensure we are
not falling into the same error pattern somewhere else.

Acked-by: Andrea Claudi <aclaudi@...hat.com>

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

Stephen, what do you think about using sprintf instead of asprintf in
these functions?
When dealing with paths, asprintf can indeed be a bit error-prone. I
can easily imagine this error pattern to happen again in the future.
If you agree, I can send a patch taking care of this.

> cheers,
> jamal
>

Andrea