[PATCH net] bpf: uninitialized variables in test code

[PATCH net] bpf: uninitialized variables in test code

[Dan Carpenter]

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(-)

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

Re: [PATCH net] bpf: uninitialized variables in test code

[Song Liu]

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
>

Re: [PATCH net] bpf: uninitialized variables in test code

[Alexei Starovoitov]

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 ?

Re: [PATCH net] bpf: uninitialized variables in test code

[Roman Gushchin]

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;
 }
-- 
2.17.2

Re: [PATCH net] bpf: uninitialized variables in test code

[Alexei Starovoitov]

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
> 

Re: [PATCH net] bpf: uninitialized variables in test code

[Roman Gushchin]

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;
 }
-- 
2.17.2

Re: [PATCH net] bpf: uninitialized variables in test code

[Alexei Starovoitov]

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!

Re: [PATCH net] bpf: uninitialized variables in test code

[Dan Carpenter]

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