On 9/6/24 2:15 AM, Alexei Starovoitov wrote:
On Thu, Sep 5, 2024 at 4:14 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
On 9/6/24 12:56 AM, Daniel Borkmann wrote:
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.
Nevermind, scratch the incorrect last part, only this option would do the
trick since from bpf pov its bpf-side "long" (as its unchanged in the uapi
header which gets pulled into the prog).
Right. From bpf side 'long *' is ok-sh and it's ok to stay this way
in uapi/bpf.h and from there in bpf_helper_defs.h,
but BPF_CALL(bpf_strol..) needs to change.
And if we fix that we should probably change uapi/bpf.h to stay consistent.
Maybe we should use 'u64 *' everywhere then?
I'd love to also change uapi/bpf.h, just that this needs changes in existing
BPF selftests as otherwise the build errors out e.g. on regular x86-64 with
the following (without the -Werror this is 'just' a warning):
[...]
progs/verifier_const.c:28:36: error: incompatible pointer types passing 'long *' to parameter of type '__s64 *' (aka 'long long *') [-Werror,-Wincompatible-pointer-types]
28 | bpf_strtol(buff, sizeof(buff), 0, &bar);
| ^~~~
[...]
One could argue that these two helpers are still fairly niche, but otoh,
they've been around since 2019. :/ So I guess it's probably better to stay
with the uapi/bpf.h inconsistency that they remain at {unsigned,} long
instead of __{u,s}64 such that user code does not need to be adapted to the
signature change.
On 32-bit archs bpf_strtol helpers were broken,
since they were converting string to 'long long', but assigning
result into 32-bit 'long *',
so upper bits will be seen as uninited from bpf prog pov.
This series are fixing the error path of uninit, but looks like
non-error path was broken on 32-bit archs too.
Thankfully bpf_strto[u]l are the only helpers that take 'long *'.
Other helpers use 'u64 *' in similar situations.
Agree, thankfully just those which slipped through..
