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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 23 Apr 2020 08:30:20 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        netdev@...r.kernel.org, dsahern@...il.com, aclaudi@...hat.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

Jamal Hadi Salim wrote on Wed, Apr 22, 2020:
> >>diff --git a/lib/bpf.c b/lib/bpf.c
> >>index 10cf9bf4..656cad02 100644
> >>--- a/lib/bpf.c
> >>+++ b/lib/bpf.c
> >>@@ -1509,15 +1509,15 @@ out:
> >>  static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
> >>  				const char *todo)
> >>  {
> >>-	char *tmp = NULL;
> >>+	char tmp[PATH_MAX] = {};
> >
> >Initializing the whole string to 0 is over kill here.
> 
> Why is it overkill? ;->
> Note: I just replicated other parts of the same file which
> initialize similar array to 0.

FWIW I kind of agree this is overkill, there's only one other occurence
of a char * being explicitely zeroed, the rest isn't strings so probably
have better reasons to.

snprintf will safely zero-terminate it and nothing should ever access
past the nul byte so it shouldn't be necessary.

> >>  	char *rem = NULL;
> >>  	char *sub;
> >>  	int ret;
> >>-	ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> >>+	ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> >
> >snprintf will never return -1.
> 
> Man page says it does. Practically it may not but we have code (in
> iproute2) which assumes it will happen.
> 
> Pick your poison:
> 1) Ignore the return code
> 2) As suggested by Dominique, something along the lines of:

(I also said I don't think it can ever fail in the non-wide-char variant
we use here (failure described in man page might be bad format string?
but we use a constant string here), and that the >= check is redundant
with the later strcat boundary checking ; by the same logic the words
you put in my mouth here are overkill as well :) (and the max size
variant would need some extra andling to set errno so check cannot be
shared that easily)
Anyway rest of iproute2 doesn't check snprintf return value much, it
should be fine to ignore)

> if (ret <= 0 || ret >= MAX_PATH)
>    ...error here..
> 
> Which one do you prefer?

-- 
Dominique