On 9/5/24 10:27 PM, Daniel Borkmann wrote:
On 9/5/24 9:39 PM, Alexei Starovoitov wrote:
On Thu, Sep 5, 2024 at 6:48 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 3956be5d6440..d2c8945e8297 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -539,7 +539,9 @@ const struct bpf_func_proto bpf_strtol_proto = {
.arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
.arg2_type = ARG_CONST_SIZE,
.arg3_type = ARG_ANYTHING,
- .arg4_type = ARG_PTR_TO_LONG,
+ .arg4_type = ARG_PTR_TO_FIXED_SIZE_MEM |
+ MEM_UNINIT | MEM_ALIGNED,
+ .arg4_size = sizeof(long),
};
BPF_CALL_4(bpf_strtoul, const char *, buf, size_t, buf_len, u64, flags,
@@ -567,7 +569,9 @@ const struct bpf_func_proto bpf_strtoul_proto = {
.arg1_type = ARG_PTR_TO_MEM | MEM_RDONLY,
.arg2_type = ARG_CONST_SIZE,
.arg3_type = ARG_ANYTHING,
- .arg4_type = ARG_PTR_TO_LONG,
+ .arg4_type = ARG_PTR_TO_FIXED_SIZE_MEM |
+ MEM_UNINIT | MEM_ALIGNED,
+ .arg4_size = sizeof(unsigned long),
This is not correct.
ARG_PTR_TO_LONG is bpf-side "long", not kernel side "long".
-static int int_ptr_type_to_size(enum bpf_arg_type type)
-{
- if (type == ARG_PTR_TO_INT)
- return sizeof(u32);
- else if (type == ARG_PTR_TO_LONG)
- return sizeof(u64);
as seen here.
BPF_CALL_4(bpf_strto[u]l, ... long *, res)
are buggy.
Right, the int_ptr_type_to_size() checks mem based on u64 vs writing
long in the helper which mismatches on 32bit archs.
but they call __bpf_strtoll which takes 'long long' correctly.
The fix for BPF_CALL_4(bpf_strto[u]l and uapi/bpf.h is orthogonal,
but this patch shouldn't make the verifier see it as sizeof(long).
Ok, so I'll fix the BPF_CALL signatures for the affected helpers as
one more patch and also align arg*_size to {s,u}64 then so that there's
no mismatch.
Not fixing up BPF_CALL signatures but aligning .arg*_size to sizeof(u64)
would fwiw keep things as today. This has the downside that on 32bit archs
one could end up leaking out 4b of uninit mem (as verifier assumes fixed
64bit and in case of write there is no need to init the var as verifier
thinks the helper will fill it all). Ugly bit is the func proto is enabled
in bpf_base_func_proto() which is ofc available for unpriv (if someone
actually has it turned on..).
Fixing up BPF_CALL signatures for bpf_strto{u,}l where res pointer becomes
{s,u}64 and .arg*_size fixed 8b, would be nicer, but assuming this includes
also the uapi helper description, then we'll also have to end up adapting
selftests (given compiler warns on ptr type mismatch) :/
One option could be we fix up BPF_CALL sites, but not the uapi helper such
that selftests stay as they are. For 64bit no change, but 32bit archs this
will be subtle as we write beyond the passed/expected long inside the helper.
Last option is to have it like in this patch to reflect actual long in
.arg*_size still no change 64bit and 32bit becomes also correct, just
quirk that it's not fixed/portable size. Thoughts on the options? All ugly,
but last one looked most sane to me fwiw.
Thanks,
Daniel
