netdev - Re: [PATCH iproute2-next] bpf: replace snprintf with asprintf when dealing with long buffers

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] [day] [month] [year] [list]

Message-ID: <8fd965a6-ece1-6fcb-4fa8-5d12f79f79e3@gmail.com>
Date:   Sun, 15 Sep 2019 11:45:24 -0600
From:   David Ahern <dsahern@...il.com>
To:     Andrea Claudi <aclaudi@...hat.com>, netdev@...r.kernel.org
Cc:     stephen@...workplumber.org, dsahern@...nel.org
Subject: Re: [PATCH iproute2-next] bpf: replace snprintf with asprintf when
 dealing with long buffers

On 9/9/19 10:05 AM, Andrea Claudi wrote:
> This reduces stack usage, as asprintf allocates memory on the heap.
> 
> This indirectly fixes a snprintf truncation warning (from gcc v9.2.1):
> 
> bpf.c: In function ‘bpf_get_work_dir’:
> bpf.c:784:49: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
>   784 |  snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
>       |                                                 ^
> bpf.c:784:2: note: ‘snprintf’ output between 2 and 4097 bytes into a destination of size 4096
>   784 |  snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Fixes: e42256699cac ("bpf: make tc's bpf loader generic and move into lib")
> Signed-off-by: Andrea Claudi <aclaudi@...hat.com>
> ---
>  lib/bpf.c | 95 +++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 57 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 7d2a322ffbaec..18e0334d3f11b 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -406,13 +406,14 @@ static int bpf_derive_elf_map_from_fdinfo(int fd, struct bpf_elf_map *map,
>  					  struct bpf_map_ext *ext)
>  {
>  	unsigned int val, owner_type = 0, owner_jited = 0;
> -	char file[PATH_MAX], buff[4096];
> +	char *file, buff[4096];
>  	FILE *fp;
>  
> -	snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
> +	asprintf(&file, "/proc/%d/fdinfo/%d", getpid(), fd);

Thanks for taking this on.

>From 'man asprintf':
  "If  memory  allocation  wasn't  possible, or some other error occurs,
these functions will return -1, and the contents of strp are undefined."

ie., you should check the return code of asprintf and make sure the
pointer passed is always initialized to NULL (static ones do not need
the init).