Re: [PATCH bpf-next] bpf: remove WARN_ON_ONCE from btf_type_id_size

[Martin KaFai Lau <martin.lau@linux.dev>]

On 10/12/22 3:04 PM, Stanislav Fomichev wrote:
> Syzkaller was able to hit it with the following reproducer:
> 
> bpf$BPF_BTF_LOAD(0x12, &(0x7f0000000140)={&(0x7f0000001680)={{0xeb9f, 0x1, 0x0, 0x18, 0x0, 0x34, 0x34, 0xc, [@fwd={0xa}, @var={0x3, 0x0, 0x0, 0x11, 0x4, 0xffffffff}, @func_proto={0x0, 0x0, 0x0, 0xd, 0x2}, @struct]}, {0x0, [0x0, 0x0, 0x5f, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x6c]}}, &(0x7f00000004c0)=""/4096, 0x58, 0x1000, 0x1}, 0x20)
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 3609 at kernel/bpf/btf.c:1946
> btf_type_id_size+0x2d5/0x9d0 kernel/bpf/btf.c:1946
> Modules linked in:
> CPU: 0 PID: 3609 Comm: syz-executor361 Not tainted
> 6.0.0-syzkaller-02734-g0326074ff465 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 09/22/2022
> RIP: 0010:btf_type_id_size+0x2d5/0x9d0 kernel/bpf/btf.c:1946
> Code: ef e8 7f 8e e4 ff 41 83 ff 0b 77 28 f6 44 24 10 18 75 3f e8 6d 91
> e4 ff 44 89 fe bf 0e 00 00 00 e8 20 8e e4 ff e8 5b 91 e4 ff <0f> 0b 45
> 31 f6 e9 98 02 00 00 41 83 ff 12 74 18 e8 46 91 e4 ff 44
> RSP: 0018:ffffc90003cefb40 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: 0000000000000002 RCX: 0000000000000000
> RDX: ffff8880259c0000 RSI: ffffffff81968415 RDI: 0000000000000005
> RBP: ffff88801270ca00 R08: 0000000000000005 R09: 000000000000000e
> R10: 0000000000000011 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000011 R14: ffff888026ee6424 R15: 0000000000000011
> FS:  000055555641b300(0000) GS:ffff8880b9a00000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000f2e258 CR3: 000000007110e000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>   <TASK>
>   btf_func_proto_check kernel/bpf/btf.c:4447 [inline]
>   btf_check_all_types kernel/bpf/btf.c:4723 [inline]
>   btf_parse_type_sec kernel/bpf/btf.c:4752 [inline]
>   btf_parse kernel/bpf/btf.c:5026 [inline]
>   btf_new_fd+0x1926/0x1e70 kernel/bpf/btf.c:6892
>   bpf_btf_load kernel/bpf/syscall.c:4324 [inline]
>   __sys_bpf+0xb7d/0x4cf0 kernel/bpf/syscall.c:5010
>   __do_sys_bpf kernel/bpf/syscall.c:5069 [inline]
>   __se_sys_bpf kernel/bpf/syscall.c:5067 [inline]
>   __x64_sys_bpf+0x75/0xb0 kernel/bpf/syscall.c:5067
>   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>   do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x7f0fbae41c69
> Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89
> f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01
> f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007ffc8aeb6228 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f0fbae41c69
> RDX: 0000000000000020 RSI: 0000000020000140 RDI: 0000000000000012
> RBP: 00007f0fbae05e10 R08: 0000000000000000 R09: 0000000000000000
> R10: 00000000ffffffff R11: 0000000000000246 R12: 00007f0fbae05ea0
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>   </TASK>
> 
> Any reason we need that WARN_ON_ONCE in this place?
> All callers except btf_array_check_member check the return value,
> so it should be safe. Assuming btf_array_check_member should also be fine
> because it hits 'btf_type_is_array()' condition.
> 
> Reported-by: syzbot+d8bd751aef7c6b39a344@syzkaller.appspotmail.com
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>   kernel/bpf/btf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index eba603cec2c5..999f62c697a7 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1943,8 +1943,8 @@ const struct btf_type *btf_type_id_size(const struct btf *btf,
>   	} else if (btf_type_is_ptr(size_type)) {
>   		size = sizeof(void *);
>   	} else {
> -		if (WARN_ON_ONCE(!btf_type_is_modifier(size_type) &&
> -				 !btf_type_is_var(size_type)))
Thanks for the report.

Trying to recall the reason...
After the above "if...else if...", the modifier and var should be the only ones 
left that may be able to resolve to a type with a size.

I suspect the type that failed the WARN_ON_ONCE here is the BTF_KIND_DECL_TAG 
which was added after this original WARN_ON_ONCE.  Could you confirm in the 
above syzkaller BTF that BTF_KIND_DECL_TAG is the one triggering here? and could 
you help to turn the above syzkaller BTF into a unittest in 
tools/testing/selftests/bpf/prog_tests/btf.c.

If that is the case, it seems we have missed checking BTF_KIND_DECL_TAG earlier 
before doing the actual btf_resolve().  The problem is in the 
btf_func_proto_check().  I talked to Yonghong offline a little on how the 
non-func-proto type is handling the invalid decl tag.  There is a 
btf_type_is_resolve_source_only() to ensure a few types (decl_tag is one of 
them) can never be referred by other types.  This check is needed in 
bpf_func_proto_check() before doing the btf_resolve().

Something like this (may not compile) and probably need similar check in the 
nr_args for loop a few lines below also:

diff --git i/kernel/bpf/btf.c w/kernel/bpf/btf.c
index eba603cec2c5..a19dbeecd2a4 100644
--- i/kernel/bpf/btf.c
+++ w/kernel/bpf/btf.c
@@ -4436,6 +4436,11 @@ static int btf_func_proto_check(struct btf_verifier_env *env,
                         return -EINVAL;
                 }

+               if (btf_type_is_resolve_source_only(ret_type)) {
+                       btf_verifier_log_type(env, t, "Invalid return type");
+                       return -EINVAL;
+               }
+
                 if (btf_type_needs_resolve(ret_type) &&
                     !env_type_is_resolved(env, ret_type_id)) {
                         err = btf_resolve(env, ret_type, ret_type_id);