| Message ID | 20181129102703.2huzlalirskjdl4k@kili.mountain |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [net] bpf: uninitialized variables in test code | expand |
On Thu, Nov 29, 2018 at 2:28 AM Dan Carpenter <dan.carpenter@oracle.com> 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. > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Acked-by: Song Liu <songliubraving@fb.com> > --- > net/bpf/test_run.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c > index c89c22c49015..49304192a031 100644 > --- a/net/bpf/test_run.c > +++ b/net/bpf/test_run.c > @@ -114,7 +114,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, > bool is_l2 = false, is_direct_pkt_access = false; > u32 size = kattr->test.data_size_in; > u32 repeat = kattr->test.repeat; > - u32 retval, duration; > + u32 retval, duration = 0; > int hh_len = ETH_HLEN; > struct sk_buff *skb; > struct sock *sk; > @@ -196,7 +196,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > u32 repeat = kattr->test.repeat; > struct netdev_rx_queue *rxqueue; > struct xdp_buff xdp = {}; > - u32 retval, duration; > + u32 retval, duration = 0; > void *data; > int ret; > > -- > 2.11.0 >
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. > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> That is incorrect fixes tag. It should be pointing to commit f42ee093be29 ("bpf/test_run: support cgroup local storage") 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). Dan, can you do such refactoring or you want to punt back to Roman ?
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. > > > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > That is incorrect fixes tag. > It should be pointing to commit f42ee093be29 ("bpf/test_run: support cgroup local storage") > > 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 <guro@fb.com> 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 <dan.carpenter@oracle.com> Suggested-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Alexei Starovoitov <ast@kernel.org> --- 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; } -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] = { 0 }; enum bpf_cgroup_storage_type stype; u64 time_start, time_spent = 0; - u32 ret = 0, i; + u32 i; for_each_cgroup_storage_type(stype) { storage[stype] = 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 = 1; time_start = ktime_get_ns(); for (i = 0; i < repeat; i++) { - ret = bpf_test_run_one(prog, ctx, storage); + *ret = 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]); - return ret; + return 0; } 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 = bpf_test_run(prog, skb, repeat, &duration); + ret = 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 = HH_DATA_ALIGN(hh_len - skb_headroom(skb)); @@ -212,11 +218,14 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); xdp.rxq = &rxqueue->xdp_rxq; - retval = bpf_test_run(prog, &xdp, repeat, &duration); + ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration); + if (ret) + goto out; if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN || xdp.data_end != xdp.data + size) size = xdp.data_end - xdp.data; ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration); +out: kfree(data); return ret; }
On Sat, Dec 01, 2018 at 07:13:50PM +0000, Roman Gushchin wrote: > 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. > > > > > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > That is incorrect fixes tag. > > It should be pointing to commit f42ee093be29 ("bpf/test_run: support cgroup local storage") > > > > 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 for the quick fix! > Thanks! > > -- > > From a2832f56c621d7809da8d4196877fa01621055f5 Mon Sep 17 00:00:00 2001 > From: Roman Gushchin <guro@fb.com> > 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 <dan.carpenter@oracle.com> > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Alexei Starovoitov <ast@kernel.org> > --- > 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; > } > > -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) may be 'int' return value? > { > struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { 0 }; > enum bpf_cgroup_storage_type stype; > u64 time_start, time_spent = 0; > - u32 ret = 0, i; > + u32 i; > > for_each_cgroup_storage_type(stype) { > storage[stype] = 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 = 1; > time_start = ktime_get_ns(); > for (i = 0; i < repeat; i++) { > - ret = bpf_test_run_one(prog, ctx, storage); > + *ret = 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]); > > - return ret; > + return 0; > } > > 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 = bpf_test_run(prog, skb, repeat, &duration); > + ret = bpf_test_run(prog, skb, repeat, &retval, &duration); > + if (ret) { > + kfree(data); should probably be kfree_skb(skb); instead ? > + kfree(sk); > + return ret; > + } > if (!is_l2) { > if (skb_headroom(skb) < hh_len) { > int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb)); > @@ -212,11 +218,14 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, > rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); > xdp.rxq = &rxqueue->xdp_rxq; > > - retval = bpf_test_run(prog, &xdp, repeat, &duration); > + ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration); > + if (ret) > + goto out; > if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN || > xdp.data_end != xdp.data + size) > size = xdp.data_end - xdp.data; > ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration); > +out: > kfree(data); > return ret; > } > -- > 2.17.2 >
On Sat, Dec 01, 2018 at 11:28:46AM -0800, Alexei Starovoitov wrote: > On Sat, Dec 01, 2018 at 07:13:50PM +0000, Roman Gushchin wrote: > > 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. > > > > > > > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > That is incorrect fixes tag. > > > It should be pointing to commit f42ee093be29 ("bpf/test_run: support cgroup local storage") > > > > > > 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 for the quick fix! > > > Thanks! > > > > -- > > > > From a2832f56c621d7809da8d4196877fa01621055f5 Mon Sep 17 00:00:00 2001 > > From: Roman Gushchin <guro@fb.com> > > 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 <dan.carpenter@oracle.com> > > Suggested-by: Alexei Starovoitov <ast@kernel.org> > > Signed-off-by: Roman Gushchin <guro@fb.com> > > Cc: Daniel Borkmann <daniel@iogearbox.net> > > Cc: Alexei Starovoitov <ast@kernel.org> > > --- > > 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; > > } > > > > -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) > > may be 'int' return value? Sure. > > > { > > struct bpf_cgroup_storage *storage[MAX_BPF_CGROUP_STORAGE_TYPE] = { 0 }; > > enum bpf_cgroup_storage_type stype; > > u64 time_start, time_spent = 0; > > - u32 ret = 0, i; > > + u32 i; > > > > for_each_cgroup_storage_type(stype) { > > storage[stype] = 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 = 1; > > time_start = ktime_get_ns(); > > for (i = 0; i < repeat; i++) { > > - ret = bpf_test_run_one(prog, ctx, storage); > > + *ret = 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]); > > > > - return ret; > > + return 0; > > } > > > > 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 = bpf_test_run(prog, skb, repeat, &duration); > > + ret = bpf_test_run(prog, skb, repeat, &retval, &duration); > > + if (ret) { > > + kfree(data); > > should probably be kfree_skb(skb); instead ? Agree. An updated version below. Thanks! -- From dc70ddb39c2f8d87d64b8d0fd71f4baa956d5f50 Mon Sep 17 00:00:00 2001 From: Roman Gushchin <guro@fb.com> Date: Sat, 1 Dec 2018 10:39:44 -0800 Subject: [PATCH v2 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 <dan.carpenter@oracle.com> Suggested-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Roman Gushchin <guro@fb.com> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: Alexei Starovoitov <ast@kernel.org> --- 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..25001913d03b 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; } -static u32 bpf_test_run(struct bpf_prog *prog, void *ctx, u32 repeat, u32 *time) +static int 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] = { 0 }; enum bpf_cgroup_storage_type stype; u64 time_start, time_spent = 0; - u32 ret = 0, i; + u32 i; for_each_cgroup_storage_type(stype) { storage[stype] = 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 = 1; time_start = ktime_get_ns(); for (i = 0; i < repeat; i++) { - ret = bpf_test_run_one(prog, ctx, storage); + *ret = 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]); - return ret; + return 0; } 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 = bpf_test_run(prog, skb, repeat, &duration); + ret = bpf_test_run(prog, skb, repeat, &retval, &duration); + if (ret) { + kfree_skb(skb); + kfree(sk); + return ret; + } if (!is_l2) { if (skb_headroom(skb) < hh_len) { int nhead = HH_DATA_ALIGN(hh_len - skb_headroom(skb)); @@ -212,11 +218,14 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, rxqueue = __netif_get_rx_queue(current->nsproxy->net_ns->loopback_dev, 0); xdp.rxq = &rxqueue->xdp_rxq; - retval = bpf_test_run(prog, &xdp, repeat, &duration); + ret = bpf_test_run(prog, &xdp, repeat, &retval, &duration); + if (ret) + goto out; if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN || xdp.data_end != xdp.data + size) size = xdp.data_end - xdp.data; ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration); +out: kfree(data); return ret; }
On Sat, Dec 01, 2018 at 08:12:34PM +0000, Roman Gushchin wrote: > > From dc70ddb39c2f8d87d64b8d0fd71f4baa956d5f50 Mon Sep 17 00:00:00 2001 > From: Roman Gushchin <guro@fb.com> > Date: Sat, 1 Dec 2018 10:39:44 -0800 > Subject: [PATCH v2 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 <dan.carpenter@oracle.com> > Suggested-by: Alexei Starovoitov <ast@kernel.org> > Signed-off-by: Roman Gushchin <guro@fb.com> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: Alexei Starovoitov <ast@kernel.org> Applied to bpf tree. Thanks!
I'm afraid Roman's patch doesn't fix the bug. 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. > > > > Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > That is incorrect fixes tag. Yeah. You're right. The Fixes tag is wrong. I spent some time looking at this too, because the code is old but the warning only just appeared... :/ Thanks for fixing this, Roman. regards, dan carpenter
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index c89c22c49015..49304192a031 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -114,7 +114,7 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr, bool is_l2 = false, is_direct_pkt_access = false; u32 size = kattr->test.data_size_in; u32 repeat = kattr->test.repeat; - u32 retval, duration; + u32 retval, duration = 0; int hh_len = ETH_HLEN; struct sk_buff *skb; struct sock *sk; @@ -196,7 +196,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, u32 repeat = kattr->test.repeat; struct netdev_rx_queue *rxqueue; struct xdp_buff xdp = {}; - u32 retval, duration; + u32 retval, duration = 0; void *data; int ret;
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. Fixes: 1cf1cae963c2 ("bpf: introduce BPF_PROG_TEST_RUN command") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- net/bpf/test_run.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)