Commit 29ebbba7 authored by Stanislav Fomichev's avatar Stanislav Fomichev Committed by Martin KaFai Lau

bpf: Don't EFAULT for {g,s}setsockopt with wrong optlen

With the way the hooks implemented right now, we have a special
condition: optval larger than PAGE_SIZE will expose only first 4k into
BPF; any modifications to the optval are ignored. If the BPF program
doesn't handle this condition by resetting optlen to 0,
the userspace will get EFAULT.

The intention of the EFAULT was to make it apparent to the
developers that the program is doing something wrong.
However, this inadvertently might affect production workloads
with the BPF programs that are not too careful (i.e., returning EFAULT
for perfectly valid setsockopt/getsockopt calls).

Let's try to minimize the chance of BPF program screwing up userspace
by ignoring the output of those BPF programs (instead of returning
EFAULT to the userspace). pr_info_once those cases to
the dmesg to help with figuring out what's going wrong.

Fixes: 0d01da6a ("bpf: implement getsockopt and setsockopt hooks")
Suggested-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: default avatarStanislav Fomichev <sdf@google.com>
Link: https://lore.kernel.org/r/20230511170456.1759459-2-sdf@google.comSigned-off-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
parent bdeeed34
...@@ -1826,6 +1826,12 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level, ...@@ -1826,6 +1826,12 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
ret = 1; ret = 1;
} else if (ctx.optlen > max_optlen || ctx.optlen < -1) { } else if (ctx.optlen > max_optlen || ctx.optlen < -1) {
/* optlen is out of bounds */ /* optlen is out of bounds */
if (*optlen > PAGE_SIZE && ctx.optlen >= 0) {
pr_info_once("bpf setsockopt: ignoring program buffer with optlen=%d (max_optlen=%d)\n",
ctx.optlen, max_optlen);
ret = 0;
goto out;
}
ret = -EFAULT; ret = -EFAULT;
} else { } else {
/* optlen within bounds, run kernel handler */ /* optlen within bounds, run kernel handler */
...@@ -1881,8 +1887,10 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level, ...@@ -1881,8 +1887,10 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
.optname = optname, .optname = optname,
.current_task = current, .current_task = current,
}; };
int orig_optlen;
int ret; int ret;
orig_optlen = max_optlen;
ctx.optlen = max_optlen; ctx.optlen = max_optlen;
max_optlen = sockopt_alloc_buf(&ctx, max_optlen, &buf); max_optlen = sockopt_alloc_buf(&ctx, max_optlen, &buf);
if (max_optlen < 0) if (max_optlen < 0)
...@@ -1905,6 +1913,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level, ...@@ -1905,6 +1913,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
ret = -EFAULT; ret = -EFAULT;
goto out; goto out;
} }
orig_optlen = ctx.optlen;
if (copy_from_user(ctx.optval, optval, if (copy_from_user(ctx.optval, optval,
min(ctx.optlen, max_optlen)) != 0) { min(ctx.optlen, max_optlen)) != 0) {
...@@ -1922,6 +1931,12 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level, ...@@ -1922,6 +1931,12 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
goto out; goto out;
if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) { if (optval && (ctx.optlen > max_optlen || ctx.optlen < 0)) {
if (orig_optlen > PAGE_SIZE && ctx.optlen >= 0) {
pr_info_once("bpf getsockopt: ignoring program buffer with optlen=%d (max_optlen=%d)\n",
ctx.optlen, max_optlen);
ret = retval;
goto out;
}
ret = -EFAULT; ret = -EFAULT;
goto out; goto out;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment