Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 21 Nov 2018 08:58:52 -0800
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Wen Yang <wen.yang99@zte.com.cn>
Cc:     ast@kernel.org, daniel@iogearbox.net, quentin.monnet@netronome.com,
        jiong.wang@netronome.com, guro@fb.com, sandipan@linux.vnet.ibm.com,
        john.fastabend@gmail.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, zhong.weidong@zte.com.cn,
        wang.yi59@zte.com.cn, Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: [PATCH 3/4] tools: bpftool: fix potential NULL pointer
 dereference in do_load
Message-ID: <20181121085852.31578bb4@cakuba.netronome.com>
In-Reply-To: <1542786192-19164-1-git-send-email-wen.yang99@zte.com.cn>
References: <1542786192-19164-1-git-send-email-wen.yang99@zte.com.cn>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@zte.com.cn>
> Reviewed-by: Tan Hu <tan.hu@zte.com.cn>
> CC: Julia Lawall <julia.lawall@lip6.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;
>  }