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

[Jamal Hadi Salim <jhs@mojatatu.com>]

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