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

Commit Message

Kui-Feng Lee Jan. 25, 2024, 11:31 p.m. UTC

From: Kui-Feng Lee <[e-mail]>

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: [e-mail]
Closes: https://lore.kernel.org/bpf/[e-mail]/
Fixes: fcc2c1fb0651 ("bpf: pass attached BTF to the bpf_struct_ops subsystem")
Signed-off-by: Kui-Feng Lee <[e-mail]>
---
 kernel/bpf/bpf_struct_ops.c | 2 ++
 kernel/bpf/verifier.c       | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Comments

Martin KaFai Lau Jan. 26, 2024, 12:54 a.m. UTC | #1

On 1/25/24 3:31 PM, [e-mail] wrote:
> From: Kui-Feng Lee <[e-mail]>
> 
> 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: [e-mail]
> Closes: https://lore.kernel.org/bpf/[e-mail]/

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 <[e-mail]>
> ---
>   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);

Kui-Feng Lee Jan. 26, 2024, 2:08 a.m. UTC | #2

On 1/25/24 16:54, Martin KaFai Lau wrote:
> On 1/25/24 3:31 PM, [e-mail] wrote:
>> From: Kui-Feng Lee <[e-mail]>
>>
>> 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: [e-mail]
>> Closes: 
>> https://lore.kernel.org/bpf/[e-mail]/
> 
> There were two different syzbot report. Both should be tagged here as 
> Reported-by.

Sure!

> 
>> Fixes: fcc2c1fb0651 ("bpf: pass attached BTF to the bpf_struct_ops 
>> subsystem")
>> Signed-off-by: Kui-Feng Lee <[e-mail]>
>> ---
>>   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?

Yes, you are right.

> 
>> +        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);
>

Patch

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)
+		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);