Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
From: Martin KaFai Lau <hidden>
Date: 2021-01-11 22:33:30
Also in:
bpf
On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Fixes: d8fe449a9c51 ("bpf: Don't return EINVAL from {get,set}sockopt when optlen > PAGE_SIZE") Signed-off-by: Stanislav Fomichev <redacted> --- 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; }