Re: [PATCH bpf-next] bpf: Fix error checks against bpf_get_btf_vmlinux().

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

On 1/25/24 3:31 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Check whether the returned pointer is NULL. Previously, it was assumed that
> an error code would be returned if BTF is not available or fails to
> parse. However, it actually returns NULL if BTF is disabled.
> 
> In the function check_struct_ops_btf_id(), we have stopped using
> btf_vmlinux as a backup because attach_btf is never null when attach_btf_id
> is set. However, the function test_libbpf_probe_prog_types() in
> libbpf_probes.c does not set both attach_btf_obj_fd and attach_btf_id,
> resulting in attach_btf being null, and it expects ENOTSUPP as a
> result. So, if attach_btf_id is not set, it will return ENOTSUPP.
> 
> Reported-by: syzbot+88f0aafe5f950d7489d7@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/bpf/00000000000040d68a060fc8db8c@google.com/

There were two different syzbot report. Both should be tagged here as Reported-by.

> Fixes: fcc2c1fb0651 ("bpf: pass attached BTF to the bpf_struct_ops subsystem")
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 2 ++
>   kernel/bpf/verifier.c       | 8 +++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index defc052e4622..0decd862dfe0 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -669,6 +669,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   		btf = bpf_get_btf_vmlinux();
>   		if (IS_ERR(btf))
>   			return ERR_CAST(btf);
> +		if (!btf)
> +			return ERR_PTR(-ENOTSUPP);
>   	}
>   
>   	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fe833e831cb6..64a927784c54 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20298,7 +20298,13 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>   		return -EINVAL;
>   	}
>   
> -	btf = prog->aux->attach_btf ?: bpf_get_btf_vmlinux();
> +	if (!prog->aux->attach_btf_id)
> +		return -ENOTSUPP;
> +
> +	btf = prog->aux->attach_btf;
> +	if (!btf)

The commit message mentioned "attach_btf is never null when attach_btf_id is 
set". Then why this test is still needed when the above has just tested the 
attach_btf_id. attach_btf must be valid here as long as attach_btf_id is set. 
This should have been guaranteed by syscall.c, no?

> +		return -ENOTSUPP;
> +
>   	if (btf_is_module(btf)) {
>   		/* Make sure st_ops is valid through the lifetime of env */
>   		env->attach_btf_mod = btf_try_get_module(btf);