* [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
@ 2021-01-11 19:47 Stanislav Fomichev
2021-01-11 22:31 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2021-01-11 19:47 UTC (permalink / raw)
To: netdev, bpf; +Cc: ast, daniel, Stanislav Fomichev, Martin KaFai Lau
optlen == 0 indicates that the kernel should ignore BPF buffer
and use the original one from the user. We, however, forget
to free the temporary buffer that we've allocated for BPF.
Reported-by: Martin KaFai Lau <kafai@fb.com>
Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
kernel/bpf/cgroup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 6ec088a96302..09179ab72c03 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1395,7 +1395,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
}
out:
- if (ret)
+ if (*kernel_optval == NULL)
sockopt_free_buf(&ctx);
return ret;
}
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
2021-01-11 19:47 [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0 Stanislav Fomichev
@ 2021-01-11 22:31 ` Martin KaFai Lau
2021-01-11 22:38 ` Stanislav Fomichev
0 siblings, 1 reply; 4+ messages in thread
From: Martin KaFai Lau @ 2021-01-11 22:31 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, bpf, ast, daniel
On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:
> optlen == 0 indicates that the kernel should ignore BPF buffer
> and use the original one from the user. We, however, forget
> to free the temporary buffer that we've allocated for BPF.
>
> Reported-by: Martin KaFai Lau <kafai@fb.com>
> Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> kernel/bpf/cgroup.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 6ec088a96302..09179ab72c03 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1395,7 +1395,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> }
>
> out:
> - if (ret)
> + if (*kernel_optval == NULL)
It seems fragile to depend on the caller to init *kernel_optval to NULL.
How about something like:
diff --git i/kernel/bpf/cgroup.c w/kernel/bpf/cgroup.c
index 6ec088a96302..8d94c004e781 100644
--- i/kernel/bpf/cgroup.c
+++ w/kernel/bpf/cgroup.c
@@ -1358,7 +1358,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
if (copy_from_user(ctx.optval, optval, min(*optlen, max_optlen)) != 0) {
ret = -EFAULT;
- goto out;
+ goto err_out;
}
lock_sock(sk);
@@ -1368,7 +1368,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
if (!ret) {
ret = -EPERM;
- goto out;
+ goto err_out;
}
if (ctx.optlen == -1) {
@@ -1379,7 +1379,6 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
ret = -EFAULT;
} else {
/* optlen within bounds, run kernel handler */
- ret = 0;
/* export any potential modifications */
*level = ctx.level;
@@ -1391,12 +1390,15 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
if (ctx.optlen != 0) {
*optlen = ctx.optlen;
*kernel_optval = ctx.optval;
+ } else {
+ sockopt_free_buf(&ctx);
}
+
+ return 0;
}
-out:
- if (ret)
- sockopt_free_buf(&ctx);
+err_out:
+ sockopt_free_buf(&ctx);
return ret;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
2021-01-11 22:31 ` Martin KaFai Lau
@ 2021-01-11 22:38 ` Stanislav Fomichev
2021-01-11 23:10 ` Martin KaFai Lau
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2021-01-11 22:38 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: Netdev, bpf, Alexei Starovoitov, Daniel Borkmann
On Mon, Jan 11, 2021 at 2:32 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:
> > optlen == 0 indicates that the kernel should ignore BPF buffer
> > and use the original one from the user. We, however, forget
> > to free the temporary buffer that we've allocated for BPF.
> >
> > Reported-by: Martin KaFai Lau <kafai@fb.com>
> > Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > kernel/bpf/cgroup.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > index 6ec088a96302..09179ab72c03 100644
> > --- a/kernel/bpf/cgroup.c
> > +++ b/kernel/bpf/cgroup.c
> > @@ -1395,7 +1395,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> > }
> >
> > out:
> > - if (ret)
> > + if (*kernel_optval == NULL)
> It seems fragile to depend on the caller to init *kernel_optval to NULL.
We can manually reset it to NULL when we enter
__cgroup_bpf_run_filter_setsockopt,
I didn't bother since there is only one existing caller.
But you patch also LGTM, I don't really have a preference.
> How about something like:
>
> diff --git i/kernel/bpf/cgroup.c w/kernel/bpf/cgroup.c
> index 6ec088a96302..8d94c004e781 100644
> --- i/kernel/bpf/cgroup.c
> +++ w/kernel/bpf/cgroup.c
> @@ -1358,7 +1358,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
>
> if (copy_from_user(ctx.optval, optval, min(*optlen, max_optlen)) != 0) {
> ret = -EFAULT;
> - goto out;
> + goto err_out;
> }
>
> lock_sock(sk);
> @@ -1368,7 +1368,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
>
> if (!ret) {
> ret = -EPERM;
> - goto out;
> + goto err_out;
> }
>
> if (ctx.optlen == -1) {
> @@ -1379,7 +1379,6 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> ret = -EFAULT;
> } else {
> /* optlen within bounds, run kernel handler */
> - ret = 0;
>
> /* export any potential modifications */
> *level = ctx.level;
> @@ -1391,12 +1390,15 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> if (ctx.optlen != 0) {
> *optlen = ctx.optlen;
> *kernel_optval = ctx.optval;
> + } else {
> + sockopt_free_buf(&ctx);
> }
> +
> + return 0;
> }
>
> -out:
> - if (ret)
> - sockopt_free_buf(&ctx);
> +err_out:
> + sockopt_free_buf(&ctx);
> return ret;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
2021-01-11 22:38 ` Stanislav Fomichev
@ 2021-01-11 23:10 ` Martin KaFai Lau
0 siblings, 0 replies; 4+ messages in thread
From: Martin KaFai Lau @ 2021-01-11 23:10 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: Netdev, bpf, Alexei Starovoitov, Daniel Borkmann
On Mon, Jan 11, 2021 at 02:38:02PM -0800, Stanislav Fomichev wrote:
> On Mon, Jan 11, 2021 at 2:32 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:
> > > optlen == 0 indicates that the kernel should ignore BPF buffer
> > > and use the original one from the user. We, however, forget
> > > to free the temporary buffer that we've allocated for BPF.
> > >
> > > Reported-by: Martin KaFai Lau <kafai@fb.com>
> > > Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE")
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > > kernel/bpf/cgroup.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> > > index 6ec088a96302..09179ab72c03 100644
> > > --- a/kernel/bpf/cgroup.c
> > > +++ b/kernel/bpf/cgroup.c
> > > @@ -1395,7 +1395,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> > > }
> > >
> > > out:
> > > - if (ret)
> > > + if (*kernel_optval == NULL)
> > It seems fragile to depend on the caller to init *kernel_optval to NULL.
> We can manually reset it to NULL when we enter
> __cgroup_bpf_run_filter_setsockopt,
It feels weird to reset the caller value at the beginning while this is not
intended to be an _init() like function, so I avoided it.
but yeah, I am fine on this way also and won't oppose it strongly ;)
> I didn't bother since there is only one existing caller.
>
> But you patch also LGTM, I don't really have a preference.
>
> > How about something like:
> >
> > diff --git i/kernel/bpf/cgroup.c w/kernel/bpf/cgroup.c
> > index 6ec088a96302..8d94c004e781 100644
> > --- i/kernel/bpf/cgroup.c
> > +++ w/kernel/bpf/cgroup.c
> > @@ -1358,7 +1358,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> >
> > if (copy_from_user(ctx.optval, optval, min(*optlen, max_optlen)) != 0) {
> > ret = -EFAULT;
> > - goto out;
> > + goto err_out;
> > }
> >
> > lock_sock(sk);
> > @@ -1368,7 +1368,7 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> >
> > if (!ret) {
> > ret = -EPERM;
> > - goto out;
> > + goto err_out;
> > }
> >
> > if (ctx.optlen == -1) {
> > @@ -1379,7 +1379,6 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> > ret = -EFAULT;
> > } else {
> > /* optlen within bounds, run kernel handler */
> > - ret = 0;
> >
> > /* export any potential modifications */
> > *level = ctx.level;
> > @@ -1391,12 +1390,15 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
> > if (ctx.optlen != 0) {
> > *optlen = ctx.optlen;
> > *kernel_optval = ctx.optval;
> > + } else {
> > + sockopt_free_buf(&ctx);
> > }
> > +
> > + return 0;
> > }
> >
> > -out:
> > - if (ret)
> > - sockopt_free_buf(&ctx);
> > +err_out:
> > + sockopt_free_buf(&ctx);
> > return ret;
> > }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-12 0:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 19:47 [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0 Stanislav Fomichev
2021-01-11 22:31 ` Martin KaFai Lau
2021-01-11 22:38 ` Stanislav Fomichev
2021-01-11 23:10 ` Martin KaFai Lau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).