Re: [PATCH bpf v2 1/2] bpf, cgroup: Assign cgroup in cgroup_sk_alloc when called from interrupt

[Tejun Heo <tj@kernel.org>]

On Mon, Sep 27, 2021 at 02:39:20PM +0200, Daniel Borkmann wrote:
> If cgroup_sk_alloc() is called from interrupt context, then just assign the
> root cgroup to skcd->cgroup. Prior to commit 8520e224f547 ("bpf, cgroups:
> Fix cgroup v2 fallback on v1/v2 mixed mode") we would just return, and later
> on in sock_cgroup_ptr(), we were NULL-testing the cgroup in fast-path, and
> iff indeed NULL returning the root cgroup (v ?: &cgrp_dfl_root.cgrp). Rather
> than re-adding the NULL-test to the fast-path we can just assign it once from
> cgroup_sk_alloc() given v1/v2 handling has been simplified. The migration from
> NULL test with returning &cgrp_dfl_root.cgrp to assigning &cgrp_dfl_root.cgrp
> directly does /not/ change behavior for callers of sock_cgroup_ptr().
> 
> syzkaller was able to trigger a splat in the legacy netrom code base, where
> the RX handler in nr_rx_frame() calls nr_make_new() which calls sk_alloc()
> and therefore cgroup_sk_alloc() with in_interrupt() condition. Thus the NULL
> skcd->cgroup, where it trips over on cgroup_sk_free() side given it expects
> a non-NULL object. There are a few other candidates aside from netrom which
> have similar pattern where in their accept-like implementation, they just call
> to sk_alloc() and thus cgroup_sk_alloc() instead of sk_clone_lock() with the
> corresponding cgroup_sk_clone() which then inherits the cgroup from the parent
> socket. None of them are related to core protocols where BPF cgroup programs
> are running from. However, in future, they should follow to implement a similar
> inheritance mechanism.
> 
> Additionally, with a !CONFIG_CGROUP_NET_PRIO and !CONFIG_CGROUP_NET_CLASSID
> configuration, the same issue was exposed also prior to 8520e224f547 due to
> commit e876ecc67db8 ("cgroup: memcg: net: do not associate sock with unrelated
> cgroup") which added the early in_interrupt() return back then.
> 
> Fixes: 8520e224f547 ("bpf, cgroups: Fix cgroup v2 fallback on v1/v2 mixed mode")
> Fixes: e876ecc67db8 ("cgroup: memcg: net: do not associate sock with unrelated cgroup")
> Reported-by: syzbot+df709157a4ecaf192b03@syzkaller.appspotmail.com
> Reported-by: syzbot+533f389d4026d86a2a95@syzkaller.appspotmail.com
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Tested-by: syzbot+df709157a4ecaf192b03@syzkaller.appspotmail.com
> Tested-by: syzbot+533f389d4026d86a2a95@syzkaller.appspotmail.com
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun