[PATCH 1/2] bpf: cgroup: Fix optlen WARN_ON_ONCE toctou

A toctou issue in `__cgroup_bpf_run_filter_getsockopt` can trigger a
WARN_ON_ONCE in a check of `copy_from_user`.
`*optlen` is checked to be non-negative in the individual getsockopt
functions beforehand. Changing `*optlen` in a race to a negative value
will result in a `copy_from_user(ctx.optval, optval, ctx.optlen)` with
`ctx.optlen` being a negative integer.

Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
Signed-off-by: Loris Reiff <redacted>
---
 kernel/bpf/cgroup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 96555a8a2..6ec8f02f4 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c

@@ -1442,6 +1442,11 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
 			goto out;
 		}
 
+		if (ctx.optlen < 0) {
+			ret = -EFAULT;
+			goto out;
+		}
+
 		if (copy_from_user(ctx.optval, optval,
 				   min(ctx.optlen, max_optlen)) != 0) {
 			ret = -EFAULT;

-- 
2.29.2

Since ctx.optlen is signed, a larger value than max_value could be
passed, as it is later on used as unsigned, which causes a WARN_ON_ONCE
in the copy_to_user.

Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
Signed-off-by: Loris Reiff <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 6ec8f02f4..6aa9e10c6 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c

@@ -1464,7 +1464,7 @@ int __cgroup_bpf_run_filter_getsockopt(struct sock *sk, int level,
 		goto out;
 	}
 
-	if (ctx.optlen > max_optlen) {
+	if (ctx.optlen > max_optlen || ctx.optlen < 0) {
 		ret = -EFAULT;
 		goto out;
 	}

-- 
2.29.2

On Fri, Jan 22, 2021 at 8:43 AM Loris Reiff [off-list ref] wrote:

Thanks! I assume this is only an issue if the BPF program is written
incorrectly.

Reviewed-by: Stanislav Fomichev <redacted>

Excerpts from Stanislav Fomichev's message of January 22, 2021 18:04:

Thanks! I assume this is only an issue if the BPF program is written
incorrectly.

Yes exactly.

On Fri, Jan 22, 2021 at 8:43 AM Loris Reiff [off-list ref] wrote:

Good point, user's optlen can be concurrently changed after the kernel
updated it.

Reviewed-by: Stanislav Fomichev <redacted>

Hello:

This series was applied to bpf/bpf.git (refs/heads/master):

On Fri, 22 Jan 2021 17:42:31 +0100 you wrote:

A toctou issue in `__cgroup_bpf_run_filter_getsockopt` can trigger a
WARN_ON_ONCE in a check of `copy_from_user`.
`*optlen` is checked to be non-negative in the individual getsockopt
functions beforehand. Changing `*optlen` in a race to a negative value
will result in a `copy_from_user(ctx.optval, optval, ctx.optlen)` with
`ctx.optlen` being a negative integer.

[...]

Here is the summary with links:
  - [1/2] bpf: cgroup: Fix optlen WARN_ON_ONCE toctou
    https://git.kernel.org/bpf/bpf/c/bb8b81e396f7
  - [2/2] bpf: cgroup: Fix problematic bounds check
    https://git.kernel.org/bpf/bpf/c/f4a2da755a7e

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html