Re: [PATCH bpf] bpf: don't leak memory in bpf getsockopt when optlen == 0
From: Stanislav Fomichev <hidden>
Date: 2021-01-11 22:39:14
Also in:
bpf
On Mon, Jan 11, 2021 at 2:32 PM Martin KaFai Lau [off-list ref] wrote:
On Mon, Jan 11, 2021 at 11:47:38AM -0800, Stanislav Fomichev wrote:quoted
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.
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.
quoted hunk ↗ jump to hunk
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; }