diff mbox series

[bpf-next,07/11] bpf: check_map_access() with the knowledge of arrays.

Message ID 20240410004150.2917641-8-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Enable BPF programs to declare arrays of kptr, bpf_rb_root, and bpf_list_head. | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
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: 955 this patch: 955
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 8 maintainers not CCed: yonghong.song@linux.dev john.fastabend@gmail.com jolsa@kernel.org kpsingh@kernel.org sdf@google.com daniel@iogearbox.net haoluo@google.com eddyz87@gmail.com
netdev/build_clang success Errors and warnings before: 955 this patch: 955
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: 966 this patch: 966
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 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
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-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
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-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
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-20 success Logs for x86_64-gcc / build-release
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 and -O2 optimization
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 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
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-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
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-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-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-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-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-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-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
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-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-10 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release

Commit Message

Kui-Feng Lee April 10, 2024, 12:41 a.m. UTC 
Ensure that accessing a map aligns with an element if the corresponding
btf_field, if there is, represents an array.

It would be necessary for an access to be aligned with the beginning of an
array if we didn't make this change. Any access to elements other than the
first one would be rejected.

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

Comments

Eduard Zingerman April 11, 2024, 10:14 p.m. UTC | #1 
On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
[...]

> Any access to elements other than the first one would be rejected.

I'm not sure this is true, could you please point me to a specific
check in the code that enforces access to go to the first element?
The check added in this patch only enforces correct alignment with
array element start.

Other than this, the patch looks good to me.

[...]

> @@ -5448,7 +5448,10 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
>  					verbose(env, "kptr access cannot have variable offset\n");
>  					return -EACCES;
>  				}
> -				if (p != off + reg->var_off.value) {
> +				var_p = off + reg->var_off.value;
> +				elem_size = field->size / field->nelems;
> +				if (var_p < p || var_p >= p + field->size ||
> +				    (var_p - p) % elem_size) {
>  					verbose(env, "kptr access misaligned expected=%u off=%llu\n",
>  						p, off + reg->var_off.value);
>  					return -EACCES;
Kui-Feng Lee April 12, 2024, 4:32 p.m. UTC | #2 
On 4/11/24 15:14, Eduard Zingerman wrote:
> On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
> [...]
> 
>> Any access to elements other than the first one would be rejected.
> 
> I'm not sure this is true, could you please point me to a specific
> check in the code that enforces access to go to the first element?
> The check added in this patch only enforces correct alignment with
> array element start.

I mean accessing to elements other than the first one would be rejected
if we don't have this patch.

Before the change, it enforces correct alignment with the start of the
whole array.  Once the array feature is enabled, the "size" of struct
btf_field will be the size of entire array. In another word, accessing
to later elements, other than the first one, doesn't align with the
beginning of entire array, and will be rejected.


> 
> Other than this, the patch looks good to me.
> 
> [...]
> 
>> @@ -5448,7 +5448,10 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
>>   					verbose(env, "kptr access cannot have variable offset\n");
>>   					return -EACCES;
>>   				}
>> -				if (p != off + reg->var_off.value) {

Here "p" is the start of the entire array. If access any elements other
than the first one, it should return -EACCES.

>> +				var_p = off + reg->var_off.value;
>> +				elem_size = field->size / field->nelems;
>> +				if (var_p < p || var_p >= p + field->size ||
>> +				    (var_p - p) % elem_size) {
>>   					verbose(env, "kptr access misaligned expected=%u off=%llu\n",
>>   						p, off + reg->var_off.value);
>>   					return -EACCES;
> 
>
Eduard Zingerman April 12, 2024, 7:08 p.m. UTC | #3 
On Fri, 2024-04-12 at 09:32 -0700, Kui-Feng Lee wrote:
> 
> On 4/11/24 15:14, Eduard Zingerman wrote:
> > On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
> > [...]
> > 
> > > Any access to elements other than the first one would be rejected.
> > 
> > I'm not sure this is true, could you please point me to a specific
> > check in the code that enforces access to go to the first element?
> > The check added in this patch only enforces correct alignment with
> > array element start.
> 
> I mean accessing to elements other than the first one would be rejected
> if we don't have this patch.

Oh, I misunderstood the above statement then.
The way I read it was: "after this patch access to elements other than
the first one would be rejected". While this patch explicitly allows
access to the subsequent array elements, hence confusion.
Sorry for the noise.

> 
> Before the change, it enforces correct alignment with the start of the
> whole array.  Once the array feature is enabled, the "size" of struct
> btf_field will be the size of entire array. In another word, accessing
> to later elements, other than the first one, doesn't align with the
> beginning of entire array, and will be rejected.
Kui-Feng Lee April 12, 2024, 7:29 p.m. UTC | #4 
On 4/12/24 12:08, Eduard Zingerman wrote:
> On Fri, 2024-04-12 at 09:32 -0700, Kui-Feng Lee wrote:
>>
>> On 4/11/24 15:14, Eduard Zingerman wrote:
>>> On Tue, 2024-04-09 at 17:41 -0700, Kui-Feng Lee wrote:
>>> [...]
>>>
>>>> Any access to elements other than the first one would be rejected.
>>>
>>> I'm not sure this is true, could you please point me to a specific
>>> check in the code that enforces access to go to the first element?
>>> The check added in this patch only enforces correct alignment with
>>> array element start.
>>
>> I mean accessing to elements other than the first one would be rejected
>> if we don't have this patch.
> 
> Oh, I misunderstood the above statement then.
> The way I read it was: "after this patch access to elements other than
> the first one would be rejected". While this patch explicitly allows
> access to the subsequent array elements, hence confusion.
> Sorry for the noise.

I will rephrase it to make it more clear.

> 
>>
>> Before the change, it enforces correct alignment with the start of the
>> whole array.  Once the array feature is enabled, the "size" of struct
>> btf_field will be the size of entire array. In another word, accessing
>> to later elements, other than the first one, doesn't align with the
>> beginning of entire array, and will be rejected.
>
Eduard Zingerman April 12, 2024, 7:50 p.m. UTC | #5 
On Fri, 2024-04-12 at 12:29 -0700, Kui-Feng Lee wrote:

[...]

> 
> > Oh, I misunderstood the above statement then.
> > The way I read it was: "after this patch access to elements other than
> > the first one would be rejected". While this patch explicitly allows
> > access to the subsequent array elements, hence confusion.
> > Sorry for the noise.
> 
> I will rephrase it to make it more clear.

Great, thank you.
>
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 34e43220c6f0..67b89d4ea1ba 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5428,7 +5428,7 @@  static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 	rec = map->record;
 	for (i = 0; i < rec->cnt; i++) {
 		struct btf_field *field = &rec->fields[i];
-		u32 p = field->offset;
+		u32 p = field->offset, var_p, elem_size;
 
 		/* If any part of a field  can be touched by load/store, reject
 		 * this program. To check that [x1, x2) overlaps with [y1, y2),
@@ -5448,7 +5448,10 @@  static int check_map_access(struct bpf_verifier_env *env, u32 regno,
 					verbose(env, "kptr access cannot have variable offset\n");
 					return -EACCES;
 				}
-				if (p != off + reg->var_off.value) {
+				var_p = off + reg->var_off.value;
+				elem_size = field->size / field->nelems;
+				if (var_p < p || var_p >= p + field->size ||
+				    (var_p - p) % elem_size) {
 					verbose(env, "kptr access misaligned expected=%u off=%llu\n",
 						p, off + reg->var_off.value);
 					return -EACCES;