netdev - Re: [PATCH 3/4] tools: bpftool: fix potential NULL pointer dereference in do

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, 21 Nov 2018 08:58:52 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Wen Yang <wen.yang99@....com.cn>
Cc:     ast@...nel.org, daniel@...earbox.net, quentin.monnet@...ronome.com,
        jiong.wang@...ronome.com, guro@...com, sandipan@...ux.vnet.ibm.com,
        john.fastabend@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, zhong.weidong@....com.cn,
        wang.yi59@....com.cn, Julia Lawall <julia.lawall@...6.fr>
Subject: Re: [PATCH 3/4] tools: bpftool: fix potential NULL pointer
 dereference in do_load

On Wed, 21 Nov 2018 15:43:12 +0800, Wen Yang wrote:
> This patch fixes a possible null pointer dereference in
> do_load, detected by the semantic patch
> deref_null.cocci, with the following warning:
> 
> ./tools/bpf/bpftool/prog.c:1021:23-25: ERROR: map_replace is NULL but dereferenced.
> 
> The following code has potential null pointer references:
>  881             map_replace = reallocarray(map_replace, old_map_fds + 1,
>  882                                        sizeof(*map_replace));
>  883             if (!map_replace) {
>  884                     p_err("mem alloc failed");
>  885                     goto err_free_reuse_maps;
>  886             }
> 
> ...
> 1019 err_free_reuse_maps:
> 1020         for (i = 0; i < old_map_fds; i++)
> 1021                 close(map_replace[i].fd);
> 1022         free(map_replace);

Ugh, good catch and very nice commit message!  However, I think the
resolution is wrong.  We still want to free the old maps.  Note that
reallocarray() does not free the old array when reallocation fails, so
we just shouldn't overwrite the map_replace with the return value.
Like this, maybe:

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 5302ee282409..be319c0eb94d 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -846,6 +846,7 @@ static int do_load(int argc, char **argv)
                        NEXT_ARG();
                } else if (is_prefix(*argv, "map")) {
                        char *endptr, *name;
+                       void *new_map_replace;
                        int fd;
 
                        NEXT_ARG();
@@ -878,12 +879,15 @@ static int do_load(int argc, char **argv)
                        if (fd < 0)
                                goto err_free_reuse_maps;
 
-                       map_replace = reallocarray(map_replace, old_map_fds + 1,
-                                                  sizeof(*map_replace));
-                       if (!map_replace) {
+                       new_map_replace = reallocarray(map_replace,
+                                                      old_map_fds + 1,
+                                                      sizeof(*map_replace));
+                       if (!new_map_replace) {
                                p_err("mem alloc failed");
                                goto err_free_reuse_maps;
                        }
+                       map_replace = new_map_replace;
+
                        map_replace[old_map_fds].idx = idx;
                        map_replace[old_map_fds].name = name;
                        map_replace[old_map_fds].fd = fd;

> Signed-off-by: Wen Yang <wen.yang99@....com.cn>
> Reviewed-by: Tan Hu <tan.hu@....com.cn>
> CC: Julia Lawall <julia.lawall@...6.fr>
> ---
>  tools/bpf/bpftool/prog.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 5302ee2..de42187 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1017,8 +1017,9 @@ static int do_load(int argc, char **argv)
>  err_close_obj:
>  	bpf_object__close(obj);
>  err_free_reuse_maps:
> -	for (i = 0; i < old_map_fds; i++)
> -		close(map_replace[i].fd);
> +	if (map_replace)
> +		for (i = 0; i < old_map_fds; i++)
> +			close(map_replace[i].fd);
>  	free(map_replace);
>  	return -1;
>  }