From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roman Gushchin Subject: Re: [PATCH net] bpf: uninitialized variables in test code Date: Sat, 1 Dec 2018 19:13:50 +0000 Message-ID: <20181201191341.GA4112@castle.DHCP.thefacebook.com> References: <20181129102703.2huzlalirskjdl4k@kili.mountain> <20181130225801.7adzjdjkn5cirq4s@ast-mbp.dhcp.thefacebook.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Dan Carpenter , Alexei Starovoitov , Daniel Borkmann , "David S. Miller" , "netdev@vger.kernel.org" , "kernel-janitors@vger.kernel.org" To: Alexei Starovoitov Return-path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:56192 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725728AbeLBG17 (ORCPT ); Sun, 2 Dec 2018 01:27:59 -0500 In-Reply-To: <20181130225801.7adzjdjkn5cirq4s@ast-mbp.dhcp.thefacebook.com> Content-Language: en-US Content-ID: <0550EA57EEC8724E9107672E22079FC5@namprd15.prod.outlook.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Nov 30, 2018 at 02:58:03PM -0800, Alexei Starovoitov wrote: > On Thu, Nov 29, 2018 at 01:27:03PM +0300, Dan Carpenter wrote: > > Smatch complains that if bpf_test_run() fails with -ENOMEM at the > > begining then the "duration" is uninitialized. We then copy the > > unintialized variables to the user inside the bpf_test_finish() > > function. The functions require CAP_SYS_ADMIN so it's not really an > > information leak. > >=20 > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > > Signed-off-by: Dan Carpenter >=20 > That is incorrect fixes tag. > It should be pointing to commit f42ee093be29 ("bpf/test_run: support cgro= up local storage") >=20 > bpf_test_run() can only return the value that bpf program returned. > It cannot return -ENOMEM. > That code needs to be refactored. > I think the proper way for bpf_test_run() would be to return 0 or -ENOMEM > and store bpf's retval into extra pointer. > Proper checks need to be added in the callers (bpf_prog_test_run_skb, etc= ). Makes total sense. How about this patch? Thanks! -- >>From a2832f56c621d7809da8d4196877fa01621055f5 Mon Sep 17 00:00:00 2001 From: Roman Gushchin Date: Sat, 1 Dec 2018 10:39:44 -0800 Subject: [PATCH bpf] bpf: refactor bpf_test_run() to separate own failures = and test program result After commit f42ee093be29 ("bpf/test_run: support cgroup local storage") the bpf_test_run() function may fail with -ENOMEM, if it's not possible to allocate memory for a cgroup local storage. This error shouldn't be mixed with the return value of the testing program. Let's add an additional argument with a pointer where to store the testing program's result; and make bpf_test_run() return either 0 or -ENOMEM. Fixes: f42ee093be29 ("bpf/test_run: support cgroup local storage") Reported-by: Dan Carpenter Suggested-by: Alexei Starovoitov Signed-off-by: Roman Gushchin Cc: Daniel Borkmann Cc: Alexei Starovoitov --- net/bpf/test_run.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index c89c22c49015..8bce7d8d00d9 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -28,12 +28,13 @@ static __always_inline u32 bpf_test_run_one(struct bpf_= prog *prog, void *ctx, return ret; } =20 -static u32 bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 = *time) +static u32 bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 = *ret, + u32 *time) { struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] =3D { 0 }= ; enum bpf_cgroup_storage_type stype; u64 time_start, time_spent =3D 0; - u32 ret =3D 0, i; + u32 i; =20 for_each_cgroup_storage_type(stype) { storage[stype] =3D bpf_cgroup_storage_alloc(prog, stype); @@ -49,7 +50,7 @@ static u32 bpf_test_run(struct bpf_prog *prog, void *ctx,= u32 repeat, u32 *time) repeat =3D 1; time_start =3D ktime_get_ns(); for (i =3D 0; i < repeat; i++) { - ret =3D bpf_test_run_one(prog, ctx, storage); + *ret =3D bpf_test_run_one(prog, ctx, storage); if (need_resched()) { if (signal_pending(current)) break; @@ -65,7 +66,7 @@ static u32 bpf_test_run(struct bpf_prog *prog, void *ctx,= u32 repeat, u32 *time) for_each_cgroup_storage_type(stype) bpf_cgroup_storage_free(storage[stype]); =20 - return ret; + return 0; } =20 static int bpf_test_finish(const union bpf_attr *kattr, @@ -165,7 +166,12 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const= union bpf_attr *kattr, __skb_push(skb, hh_len); if (is_direct_pkt_access) bpf_compute_data_pointers(skb); - retval =3D bpf_test_run(prog, skb, repeat, &duration); + ret =3D bpf_test_run(prog, skb, repeat, &retval, &duration); + if (ret) { + kfree(data); + kfree(sk); + return ret; + } if (!is_l2) { if (skb_headroom(skb) < hh_len) { int nhead =3D HH_DATA_ALIGN(hh_len - skb_headroom(skb)); @@ -212,11 +218,14 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, cons= t union bpf_attr *kattr, rxqueue =3D __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, = 0); xdp.rxq =3D &rxqueue->xdp_rxq; =20 - retval =3D bpf_test_run(prog, &xdp, repeat, &duration); + ret =3D bpf_test_run(prog, &xdp, repeat, &retval, &duration); + if (ret) + goto out; if (xdp.data !=3D data + XDP_PACKET_HEADROOM + NET_IP_ALIGN || xdp.data_end !=3D xdp.data + size) size =3D xdp.data_end - xdp.data; ret =3D bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration); +out: kfree(data); return ret; } --=20 2.17.2