diff mbox series

[RFC,bpf-next,2/5] bpf: Handle BPF_KPTR_USER in verifier.

Message ID 20240807235755.1435806-3-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Share user memory to BPF program through task storage map. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 8 maintainers not CCed: kpsingh@kernel.org haoluo@google.com daniel@iogearbox.net john.fastabend@gmail.com jolsa@kernel.org yonghong.song@linux.dev eddyz87@gmail.com sdf@fomichev.me
netdev/build_clang success Errors and warnings before: 43 this patch: 43
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 53 this patch: 53
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kui-Feng Lee Aug. 7, 2024, 11:57 p.m. UTC 
Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
to the memory pointed by it readable and writable.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/verifier.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Alexei Starovoitov Aug. 12, 2024, 4:48 p.m. UTC | #1 
On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
> to the memory pointed by it readable and writable.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  kernel/bpf/verifier.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df3be12096cf..84647e599595 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5340,6 +5340,10 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
>         int perm_flags;
>         const char *reg_name = "";
>
> +       if (kptr_field->type == BPF_KPTR_USER)
> +               /* BPF programs should not change any user kptr */
> +               return -EACCES;
> +
>         if (btf_is_kernel(reg->btf)) {
>                 perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
>
> @@ -5483,6 +5487,12 @@ static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
>                         ret |= NON_OWN_REF;
>         } else {
>                 ret |= PTR_UNTRUSTED;
> +               if (kptr_field->type == BPF_KPTR_USER)
> +                       /* In oder to access directly from bpf
> +                        * programs. NON_OWN_REF make the memory
> +                        * writable. Check check_ptr_to_btf_access().
> +                        */
> +                       ret |= MEM_ALLOC | NON_OWN_REF;

UNTRUSTED | MEM_ALLOC | NON_OWN_REF ?!

That doesn't fit into any of the existing verifier schemes.
I cannot make sense of this part.

UNTRUSTED | MEM_ALLOC is read only through exceptions logic.
The uptr has to be read/write through normal load/store.
Kui-Feng Lee Aug. 13, 2024, 4:52 p.m. UTC | #2 
On 8/12/24 09:48, Alexei Starovoitov wrote:
> On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
>> to the memory pointed by it readable and writable.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/verifier.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index df3be12096cf..84647e599595 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -5340,6 +5340,10 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
>>          int perm_flags;
>>          const char *reg_name = "";
>>
>> +       if (kptr_field->type == BPF_KPTR_USER)
>> +               /* BPF programs should not change any user kptr */
>> +               return -EACCES;
>> +
>>          if (btf_is_kernel(reg->btf)) {
>>                  perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
>>
>> @@ -5483,6 +5487,12 @@ static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
>>                          ret |= NON_OWN_REF;
>>          } else {
>>                  ret |= PTR_UNTRUSTED;
>> +               if (kptr_field->type == BPF_KPTR_USER)
>> +                       /* In oder to access directly from bpf
>> +                        * programs. NON_OWN_REF make the memory
>> +                        * writable. Check check_ptr_to_btf_access().
>> +                        */
>> +                       ret |= MEM_ALLOC | NON_OWN_REF;
> 
> UNTRUSTED | MEM_ALLOC | NON_OWN_REF ?!
> 
> That doesn't fit into any of the existing verifier schemes.
> I cannot make sense of this part.
> 
> UNTRUSTED | MEM_ALLOC is read only through exceptions logic.
> The uptr has to be read/write through normal load/store.

I will remove UNTRUSTED and leave MEM_ALLOC and NON_OWN_REF.
Does it make sense to you?
Alexei Starovoitov Aug. 13, 2024, 7:35 p.m. UTC | #3 
On Tue, Aug 13, 2024 at 9:52 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 8/12/24 09:48, Alexei Starovoitov wrote:
> > On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
> >>
> >> Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
> >> to the memory pointed by it readable and writable.
> >>
> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> >> ---
> >>   kernel/bpf/verifier.c | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index df3be12096cf..84647e599595 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -5340,6 +5340,10 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
> >>          int perm_flags;
> >>          const char *reg_name = "";
> >>
> >> +       if (kptr_field->type == BPF_KPTR_USER)
> >> +               /* BPF programs should not change any user kptr */
> >> +               return -EACCES;
> >> +
> >>          if (btf_is_kernel(reg->btf)) {
> >>                  perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
> >>
> >> @@ -5483,6 +5487,12 @@ static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
> >>                          ret |= NON_OWN_REF;
> >>          } else {
> >>                  ret |= PTR_UNTRUSTED;
> >> +               if (kptr_field->type == BPF_KPTR_USER)
> >> +                       /* In oder to access directly from bpf
> >> +                        * programs. NON_OWN_REF make the memory
> >> +                        * writable. Check check_ptr_to_btf_access().
> >> +                        */
> >> +                       ret |= MEM_ALLOC | NON_OWN_REF;
> >
> > UNTRUSTED | MEM_ALLOC | NON_OWN_REF ?!
> >
> > That doesn't fit into any of the existing verifier schemes.
> > I cannot make sense of this part.
> >
> > UNTRUSTED | MEM_ALLOC is read only through exceptions logic.
> > The uptr has to be read/write through normal load/store.
>
> I will remove UNTRUSTED and leave MEM_ALLOC and NON_OWN_REF.
> Does it make sense to you?

I don't think it fits either.
MEM_ALLOC | NON_OWN_REF is specific to bpf_rbtree/linklist nodes.
There are various checks and logic like:
1.
      if (!(type_is_ptr_alloc_obj(reg->type) ||
type_is_non_owning_ref(reg->type)) &&
            WARN_ON_ONCE(reg->off))
          return;
2.
invalidate_non_owning_refs() during unlock

that shouldn't apply in this case.

PTR_TO_MEM with specific mem_size fits better.
Since it's user/kernel shared memory PTR_TO_BTF_ID logic with field walking
won't work anyway, so opaque array of bytes is better. Which is PTR_TO_MEM.
Kui-Feng Lee Aug. 13, 2024, 11:13 p.m. UTC | #4 
On 8/13/24 12:35, Alexei Starovoitov wrote:
> On Tue, Aug 13, 2024 at 9:52 AM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 8/12/24 09:48, Alexei Starovoitov wrote:
>>> On Wed, Aug 7, 2024 at 4:58 PM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>>>
>>>> Give PTR_MAYBE_NULL | PTR_UNTRUSTED | MEM_ALLOC | NON_OWN_REF to kptr_user
>>>> to the memory pointed by it readable and writable.
>>>>
>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>> ---
>>>>    kernel/bpf/verifier.c | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index df3be12096cf..84647e599595 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -5340,6 +5340,10 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
>>>>           int perm_flags;
>>>>           const char *reg_name = "";
>>>>
>>>> +       if (kptr_field->type == BPF_KPTR_USER)
>>>> +               /* BPF programs should not change any user kptr */
>>>> +               return -EACCES;
>>>> +
>>>>           if (btf_is_kernel(reg->btf)) {
>>>>                   perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
>>>>
>>>> @@ -5483,6 +5487,12 @@ static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
>>>>                           ret |= NON_OWN_REF;
>>>>           } else {
>>>>                   ret |= PTR_UNTRUSTED;
>>>> +               if (kptr_field->type == BPF_KPTR_USER)
>>>> +                       /* In oder to access directly from bpf
>>>> +                        * programs. NON_OWN_REF make the memory
>>>> +                        * writable. Check check_ptr_to_btf_access().
>>>> +                        */
>>>> +                       ret |= MEM_ALLOC | NON_OWN_REF;
>>>
>>> UNTRUSTED | MEM_ALLOC | NON_OWN_REF ?!
>>>
>>> That doesn't fit into any of the existing verifier schemes.
>>> I cannot make sense of this part.
>>>
>>> UNTRUSTED | MEM_ALLOC is read only through exceptions logic.
>>> The uptr has to be read/write through normal load/store.
>>
>> I will remove UNTRUSTED and leave MEM_ALLOC and NON_OWN_REF.
>> Does it make sense to you?
> 
> I don't think it fits either.
> MEM_ALLOC | NON_OWN_REF is specific to bpf_rbtree/linklist nodes.
> There are various checks and logic like:
> 1.
>        if (!(type_is_ptr_alloc_obj(reg->type) ||
> type_is_non_owning_ref(reg->type)) &&
>              WARN_ON_ONCE(reg->off))
>            return;
> 2.
> invalidate_non_owning_refs() during unlock
> 
> that shouldn't apply in this case.
> 
> PTR_TO_MEM with specific mem_size fits better.
> Since it's user/kernel shared memory PTR_TO_BTF_ID logic with field walking
> won't work anyway, so opaque array of bytes is better. Which is PTR_TO_MEM.

Make sense!
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index df3be12096cf..84647e599595 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5340,6 +5340,10 @@  static int map_kptr_match_type(struct bpf_verifier_env *env,
 	int perm_flags;
 	const char *reg_name = "";
 
+	if (kptr_field->type == BPF_KPTR_USER)
+		/* BPF programs should not change any user kptr */
+		return -EACCES;
+
 	if (btf_is_kernel(reg->btf)) {
 		perm_flags = PTR_MAYBE_NULL | PTR_TRUSTED | MEM_RCU;
 
@@ -5483,6 +5487,12 @@  static u32 btf_ld_kptr_type(struct bpf_verifier_env *env, struct btf_field *kptr
 			ret |= NON_OWN_REF;
 	} else {
 		ret |= PTR_UNTRUSTED;
+		if (kptr_field->type == BPF_KPTR_USER)
+			/* In oder to access directly from bpf
+			 * programs. NON_OWN_REF make the memory
+			 * writable. Check check_ptr_to_btf_access().
+			 */
+			ret |= MEM_ALLOC | NON_OWN_REF;
 	}
 
 	return ret;
@@ -5576,6 +5586,7 @@  static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 			case BPF_KPTR_UNREF:
 			case BPF_KPTR_REF:
 			case BPF_KPTR_PERCPU:
+			case BPF_KPTR_USER:
 				if (src != ACCESS_DIRECT) {
 					verbose(env, "kptr cannot be accessed indirectly by helper\n");
 					return -EACCES;