* [PATCH bpf-next v2 1/3] bpf: make reg_not_null() true for CONST_PTR_TO_MAP
@ 2025-06-04 0:37 Ihor Solodrai
2025-06-04 0:37 ` [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test Ihor Solodrai
2025-06-04 0:37 ` [PATCH bpf-next v2 3/3] selftests/bpf: add test cases with CONST_PTR_TO_MAP null checks Ihor Solodrai
0 siblings, 2 replies; 7+ messages in thread
From: Ihor Solodrai @ 2025-06-04 0:37 UTC (permalink / raw)
To: andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
When reg->type is CONST_PTR_TO_MAP, it can not be null. However the
verifier explores the branches under rX == 0 in check_cond_jmp_op()
even if reg->type is CONST_PTR_TO_MAP, because it was not checked for
in reg_not_null().
Fix this by adding CONST_PTR_TO_MAP to the set of types that are
considered non nullable in reg_not_null().
An old "unpriv: cmp map pointer with zero" selftest fails with this
change, because now early out correctly triggers in
check_cond_jmp_op(), making the verification to pass.
In practice verifier may allow pointer to null comparison in unpriv,
since in many cases the relevant branch and comparison op are removed
as dead code. So change the expected test result to __success_unpriv.
Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
---
kernel/bpf/verifier.c | 3 ++-
tools/testing/selftests/bpf/progs/verifier_unpriv.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a7d6e0c5928b..0c100e430744 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -405,7 +405,8 @@ static bool reg_not_null(const struct bpf_reg_state *reg)
type == PTR_TO_MAP_KEY ||
type == PTR_TO_SOCK_COMMON ||
(type == PTR_TO_BTF_ID && is_trusted_reg(reg)) ||
- type == PTR_TO_MEM;
+ type == PTR_TO_MEM ||
+ type == CONST_PTR_TO_MAP;
}
static struct btf_record *reg_btf_record(const struct bpf_reg_state *reg)
diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index a4a5e2071604..28200f068ce5 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -619,7 +619,7 @@ __naked void pass_pointer_to_tail_call(void)
SEC("socket")
__description("unpriv: cmp map pointer with zero")
-__success __failure_unpriv __msg_unpriv("R1 pointer comparison")
+__success __success_unpriv
__retval(0)
__naked void cmp_map_pointer_with_zero(void)
{
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test
2025-06-04 0:37 [PATCH bpf-next v2 1/3] bpf: make reg_not_null() true for CONST_PTR_TO_MAP Ihor Solodrai
@ 2025-06-04 0:37 ` Ihor Solodrai
2025-06-04 16:44 ` Ihor Solodrai
2025-06-04 0:37 ` [PATCH bpf-next v2 3/3] selftests/bpf: add test cases with CONST_PTR_TO_MAP null checks Ihor Solodrai
1 sibling, 1 reply; 7+ messages in thread
From: Ihor Solodrai @ 2025-06-04 0:37 UTC (permalink / raw)
To: andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
BPF program with this code must not pass verification in unpriv.
Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
---
.../selftests/bpf/progs/verifier_unpriv.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index 28200f068ce5..85b41f927272 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -634,6 +634,23 @@ l0_%=: r0 = 0; \
: __clobber_all);
}
+SEC("socket")
+__description("unpriv: cmp map pointer with const")
+__success __failure_unpriv __msg_unpriv("R1 pointer comparison prohibited")
+__retval(0)
+__naked void cmp_map_pointer_with_const(void)
+{
+ asm volatile (" \
+ r1 = 0; \
+ r1 = %[map_hash_8b] ll; \
+ if r1 == 0xcafefeeddeadbeef goto l0_%=; \
+l0_%=: r0 = 0; \
+ exit; \
+" :
+ : __imm_addr(map_hash_8b)
+ : __clobber_all);
+}
+
SEC("socket")
__description("unpriv: write into frame pointer")
__failure __msg("frame pointer is read only")
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 3/3] selftests/bpf: add test cases with CONST_PTR_TO_MAP null checks
2025-06-04 0:37 [PATCH bpf-next v2 1/3] bpf: make reg_not_null() true for CONST_PTR_TO_MAP Ihor Solodrai
2025-06-04 0:37 ` [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test Ihor Solodrai
@ 2025-06-04 0:37 ` Ihor Solodrai
1 sibling, 0 replies; 7+ messages in thread
From: Ihor Solodrai @ 2025-06-04 0:37 UTC (permalink / raw)
To: andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
A test requires the following to happen:
* CONST_PTR_TO_MAP value is put on the stack
* then this value is checked for null
* the code in the null branch fails verification
I was able to achieve this by using a stack allocated array of maps,
populated with values from a global map. This is the first test case:
map_ptr_is_never_null.
The second test case (map_ptr_is_never_null_rb) involves an array of
ringbufs and attempts to recreate a common coding pattern [1].
[1] https://lore.kernel.org/bpf/CAEf4BzZNU0gX_sQ8k8JaLe1e+Veth3Rk=4x7MDhv=hQxvO8EDw@mail.gmail.com/
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
---
.../selftests/bpf/progs/verifier_map_in_map.c | 77 +++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_map_in_map.c b/tools/testing/selftests/bpf/progs/verifier_map_in_map.c
index 7d088ba99ea5..1dd5c6902c53 100644
--- a/tools/testing/selftests/bpf/progs/verifier_map_in_map.c
+++ b/tools/testing/selftests/bpf/progs/verifier_map_in_map.c
@@ -139,4 +139,81 @@ __naked void on_the_inner_map_pointer(void)
: __clobber_all);
}
+SEC("socket")
+int map_ptr_is_never_null(void *ctx)
+{
+ struct bpf_map *maps[2] = { 0 };
+ struct bpf_map *map = NULL;
+ int __attribute__((aligned(8))) key = 0;
+
+ for (key = 0; key < 2; key++) {
+ map = bpf_map_lookup_elem(&map_in_map, &key);
+ if (map)
+ maps[key] = map;
+ else
+ return 0;
+ }
+
+ /* After the loop every element of maps is CONST_PTR_TO_MAP so
+ * the invalid branch should not be explored by the verifier.
+ */
+ if (!maps[0])
+ asm volatile ("r10 = 0;");
+
+ return 0;
+}
+
+struct {
+ __uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
+ __uint(max_entries, 1);
+ __type(key, int);
+ __type(value, int);
+ __array(values, struct {
+ __uint(type, BPF_MAP_TYPE_RINGBUF);
+ __uint(max_entries, 4096);
+ });
+} rb_in_map SEC(".maps");
+
+struct rb_ctx {
+ void *rb;
+ struct bpf_dynptr dptr;
+};
+
+static __always_inline struct rb_ctx __rb_event_reserve(__u32 sz)
+{
+ struct rb_ctx rb_ctx = {};
+ void *rb;
+ __u32 cpu = bpf_get_smp_processor_id();
+ __u32 rb_slot = cpu & 1;
+
+ rb = bpf_map_lookup_elem(&rb_in_map, &rb_slot);
+ if (!rb)
+ return rb_ctx;
+
+ rb_ctx.rb = rb;
+ bpf_ringbuf_reserve_dynptr(rb, sz, 0, &rb_ctx.dptr);
+
+ return rb_ctx;
+}
+
+static __noinline void __rb_event_submit(struct rb_ctx *ctx)
+{
+ if (!ctx->rb)
+ return;
+
+ /* If the verifier (incorrectly) concludes that ctx->rb can be
+ * NULL at this point, we'll get "BPF_EXIT instruction in main
+ * prog would lead to reference leak" error
+ */
+ bpf_ringbuf_submit_dynptr(&ctx->dptr, 0);
+}
+
+SEC("socket")
+int map_ptr_is_never_null_rb(void *ctx)
+{
+ struct rb_ctx event_ctx = __rb_event_reserve(256);
+ __rb_event_submit(&event_ctx);
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
--
2.47.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test
2025-06-04 0:37 ` [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test Ihor Solodrai
@ 2025-06-04 16:44 ` Ihor Solodrai
2025-06-04 20:42 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Solodrai @ 2025-06-04 16:44 UTC (permalink / raw)
To: andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
On 6/3/25 5:37 PM, Ihor Solodrai wrote:
> Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
> BPF program with this code must not pass verification in unpriv.
>
> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
> ---
> .../selftests/bpf/progs/verifier_unpriv.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
> index 28200f068ce5..85b41f927272 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
> @@ -634,6 +634,23 @@ l0_%=: r0 = 0; \
> : __clobber_all);
> }
>
> +SEC("socket")
> +__description("unpriv: cmp map pointer with const")
> +__success __failure_unpriv __msg_unpriv("R1 pointer comparison prohibited")
> +__retval(0)
> +__naked void cmp_map_pointer_with_const(void)
> +{
> + asm volatile (" \
> + r1 = 0; \
> + r1 = %[map_hash_8b] ll; \
> + if r1 == 0xcafefeeddeadbeef goto l0_%=; \
GCC BPF caught (correctly) that this is not a valid instruction because
imm is supposed to be 32bit [1]:
progs/verifier_unpriv.c: Assembler messages:
progs/verifier_unpriv.c:643: Error: immediate out of range, shall
fit in 32 bits
make: *** [Makefile:751:
/tmp/work/bpf/bpf/src/tools/testing/selftests/bpf/bpf_gcc/verifier_unpriv.bpf.o]
Error 1
But LLVM 20 let it compile and the test passes. I wonder whether it's a
bug in LLVM worth reporting?
[1]
https://github.com/kernel-patches/bpf/actions/runs/15430930573/job/43428666342
> +l0_%=: r0 = 0; \
> + exit; \
> +" :
> + : __imm_addr(map_hash_8b)
> + : __clobber_all);
> +}
> +
> SEC("socket")
> __description("unpriv: write into frame pointer")
> __failure __msg("frame pointer is read only")
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test
2025-06-04 16:44 ` Ihor Solodrai
@ 2025-06-04 20:42 ` Yonghong Song
2025-06-04 20:58 ` Ihor Solodrai
0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2025-06-04 20:42 UTC (permalink / raw)
To: Ihor Solodrai, andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
On 6/4/25 9:44 AM, Ihor Solodrai wrote:
> On 6/3/25 5:37 PM, Ihor Solodrai wrote:
>> Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
>> BPF program with this code must not pass verification in unpriv.
>>
>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>> ---
>> .../selftests/bpf/progs/verifier_unpriv.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>> b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>> index 28200f068ce5..85b41f927272 100644
>> --- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>> +++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>> @@ -634,6 +634,23 @@ l0_%=: r0 = 0; \
>> : __clobber_all);
>> }
>> +SEC("socket")
>> +__description("unpriv: cmp map pointer with const")
>> +__success __failure_unpriv __msg_unpriv("R1 pointer comparison
>> prohibited")
>> +__retval(0)
>> +__naked void cmp_map_pointer_with_const(void)
>> +{
>> + asm volatile (" \
>> + r1 = 0; \
>> + r1 = %[map_hash_8b] ll; \
>> + if r1 == 0xcafefeeddeadbeef goto l0_%=; \
>
> GCC BPF caught (correctly) that this is not a valid instruction
> because imm is supposed to be 32bit [1]:
>
> progs/verifier_unpriv.c: Assembler messages:
> progs/verifier_unpriv.c:643: Error: immediate out of range, shall
> fit in 32 bits
> make: *** [Makefile:751:
> /tmp/work/bpf/bpf/src/tools/testing/selftests/bpf/bpf_gcc/verifier_unpriv.bpf.o]
> Error 1
>
> But LLVM 20 let it compile and the test passes. I wonder whether it's
> a bug in LLVM worth reporting?
>
> [1]
> https://github.com/kernel-patches/bpf/actions/runs/15430930573/job/43428666342
This is a missed case for llvm. See:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp#L82-L85
Basically for the following code,
unsigned BPFMCCodeEmitter::getMachineOpValue(const MCInst &MI,
const MCOperand &MO,
SmallVectorImpl<MCFixup> &Fixups,
const MCSubtargetInfo &STI) const {
if (MO.isReg())
return MRI.getEncodingValue(MO.getReg());
if (MO.isImm())
return static_cast<unsigned>(MO.getImm());
For 'static_cast<unsigned>(MO.getImm())', MO.getImm() value is a s64, so casting to u32 should check
the value range and we didn't check them, hence didn't report an error.
The following is the fix:
if (MO.isReg())
return MRI.getEncodingValue(MO.getReg());
- if (MO.isImm())
+ if (MO.isImm()) {
+ assert(MO.getImm() >= INT_MIN && MO.getImm() <= INT_MAX);
return static_cast<unsigned>(MO.getImm());
+ }
With the above, if the clang build enables assertion, the following dump will show up:
clang: /home/yhs/work/llvm-project/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp:86: unsigned int (anonymous namespace)::BPFMCCodeEmitter::getMachin
eOpValue(const MCInst &, const MCOperand &, SmallVectorImpl<MCFixup> &, const MCSubtargetInfo &) const: Assertion `MO.getImm() >= INT_MIN && MO.getImm() <=
INT_MAX' failed.
Although llvm tends to use 'assert' a lot (and 'assert' thing will become noop on production
build), e.g.,
[~/work/llvm-project/llvm/lib/Target/BPF/MCTargetDesc (release/19.x)]$ grep assert *.cpp
BPFAsmBackend.cpp:#include <cassert>
BPFAsmBackend.cpp: assert(unsigned(Kind - FirstTargetFixupKind) < getNumFixupKinds() &&
BPFAsmBackend.cpp: assert(Value <= UINT32_MAX);
BPFAsmBackend.cpp: assert(Fixup.getKind() == FK_PCRel_2);
BPFELFObjectWriter.cpp: assert(SectionELF && "Null section for reloc symbol");
BPFInstPrinter.cpp: assert(Kind == MCSymbolRefExpr::VK_None);
BPFInstPrinter.cpp: assert((Modifier == nullptr || Modifier[0] == 0) && "No modifiers supported");
BPFInstPrinter.cpp: assert(Op.isExpr() && "Expected an expression");
BPFInstPrinter.cpp: assert(RegOp.isReg() && "Register operand not a register");
BPFInstPrinter.cpp: assert(0 && "Expected an immediate");
BPFMCCodeEmitter.cpp:#include <cassert>
BPFMCCodeEmitter.cpp: assert(MO.isExpr());
BPFMCCodeEmitter.cpp: assert(Expr->getKind() == MCExpr::SymbolRef);
BPFMCCodeEmitter.cpp: assert(Op1.isReg() && "First operand is not register.");
BPFMCCodeEmitter.cpp: assert(Op2.isImm() && "Second operand is not immediate.");
Production build tends not to enable assertion for performance reason, so
I guess we could emit an error to user for such cases. Will fix in llvm21.
>
>> +l0_%=: r0 = 0; \
>> + exit; \
>> +" :
>> + : __imm_addr(map_hash_8b)
>> + : __clobber_all);
>> +}
>> +
>> SEC("socket")
>> __description("unpriv: write into frame pointer")
>> __failure __msg("frame pointer is read only")
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test
2025-06-04 20:42 ` Yonghong Song
@ 2025-06-04 20:58 ` Ihor Solodrai
2025-06-05 15:25 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Ihor Solodrai @ 2025-06-04 20:58 UTC (permalink / raw)
To: Yonghong Song, andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
On 6/4/25 1:42 PM, Yonghong Song wrote:
>
>
> On 6/4/25 9:44 AM, Ihor Solodrai wrote:
>> On 6/3/25 5:37 PM, Ihor Solodrai wrote:
>>> Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
>>> BPF program with this code must not pass verification in unpriv.
>>>
>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>> ---
>>> .../selftests/bpf/progs/verifier_unpriv.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/
>>> tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>> index 28200f068ce5..85b41f927272 100644
>>> --- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>> +++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>> @@ -634,6 +634,23 @@ l0_%=: r0 = 0; \
>>> : __clobber_all);
>>> }
>>> +SEC("socket")
>>> +__description("unpriv: cmp map pointer with const")
>>> +__success __failure_unpriv __msg_unpriv("R1 pointer comparison
>>> prohibited")
>>> +__retval(0)
>>> +__naked void cmp_map_pointer_with_const(void)
>>> +{
>>> + asm volatile (" \
>>> + r1 = 0; \
>>> + r1 = %[map_hash_8b] ll; \
>>> + if r1 == 0xcafefeeddeadbeef goto l0_%=; \
>>
>> GCC BPF caught (correctly) that this is not a valid instruction
>> because imm is supposed to be 32bit [1]:
>>
>> progs/verifier_unpriv.c: Assembler messages:
>> progs/verifier_unpriv.c:643: Error: immediate out of range, shall
>> fit in 32 bits
>> make: *** [Makefile:751: /tmp/work/bpf/bpf/src/tools/testing/
>> selftests/bpf/bpf_gcc/verifier_unpriv.bpf.o] Error 1
>>
>> But LLVM 20 let it compile and the test passes. I wonder whether it's
>> a bug in LLVM worth reporting?
>>
>> [1] https://github.com/kernel-patches/bpf/actions/runs/15430930573/
>> job/43428666342
>
> This is a missed case for llvm. See:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/
> MCTargetDesc/BPFMCCodeEmitter.cpp#L82-L85
> Basically for the following code,
>
> unsigned BPFMCCodeEmitter::getMachineOpValue(const MCInst &MI,
> const MCOperand &MO,
> SmallVectorImpl<MCFixup>
> &Fixups,
> const MCSubtargetInfo
> &STI) const {
> if (MO.isReg())
> return MRI.getEncodingValue(MO.getReg());
> if (MO.isImm())
> return static_cast<unsigned>(MO.getImm());
>
> For 'static_cast<unsigned>(MO.getImm())', MO.getImm() value is a s64, so
> casting to u32 should check
> the value range and we didn't check them, hence didn't report an error.
I see. Out of curiosity I looked at llvm-objdump and indeed only lower
32 bits are in the binary:
0000000000000320 <cmp_map_pointer_with_const>:
100: b7 01 00 00 00 00 00 00 r1 = 0x0
101: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 = 0x0 ll
103: 15 01 00 00 ef be ad de if r1 == -0x21524111 goto +0x0
<l0_11>
Thanks for checking, Yonghong.
>
> The following is the fix:
> if (MO.isReg())
> return MRI.getEncodingValue(MO.getReg());
> - if (MO.isImm())
> + if (MO.isImm()) {
> + assert(MO.getImm() >= INT_MIN && MO.getImm() <= INT_MAX);
> return static_cast<unsigned>(MO.getImm());
> + }
>
> With the above, if the clang build enables assertion, the following dump
> will show up:
>
> clang: /home/yhs/work/llvm-project/llvm/lib/Target/BPF/MCTargetDesc/
> BPFMCCodeEmitter.cpp:86: unsigned int (anonymous
> namespace)::BPFMCCodeEmitter::getMachin
> eOpValue(const MCInst &, const MCOperand &, SmallVectorImpl<MCFixup> &,
> const MCSubtargetInfo &) const: Assertion `MO.getImm() >= INT_MIN &&
> MO.getImm() <=
> INT_MAX' failed.
>
> Although llvm tends to use 'assert' a lot (and 'assert' thing will
> become noop on production
> build), e.g.,
>
> [~/work/llvm-project/llvm/lib/Target/BPF/MCTargetDesc (release/19.x)]$
> grep assert *.cpp
> BPFAsmBackend.cpp:#include <cassert>
> BPFAsmBackend.cpp: assert(unsigned(Kind - FirstTargetFixupKind) <
> getNumFixupKinds() &&
> BPFAsmBackend.cpp: assert(Value <= UINT32_MAX);
> BPFAsmBackend.cpp: assert(Fixup.getKind() == FK_PCRel_2);
> BPFELFObjectWriter.cpp: assert(SectionELF && "Null section for
> reloc symbol");
> BPFInstPrinter.cpp: assert(Kind == MCSymbolRefExpr::VK_None);
> BPFInstPrinter.cpp: assert((Modifier == nullptr || Modifier[0] == 0) &&
> "No modifiers supported");
> BPFInstPrinter.cpp: assert(Op.isExpr() && "Expected an expression");
> BPFInstPrinter.cpp: assert(RegOp.isReg() && "Register operand not a
> register");
> BPFInstPrinter.cpp: assert(0 && "Expected an immediate");
> BPFMCCodeEmitter.cpp:#include <cassert>
> BPFMCCodeEmitter.cpp: assert(MO.isExpr());
> BPFMCCodeEmitter.cpp: assert(Expr->getKind() == MCExpr::SymbolRef);
> BPFMCCodeEmitter.cpp: assert(Op1.isReg() && "First operand is not
> register.");
> BPFMCCodeEmitter.cpp: assert(Op2.isImm() && "Second operand is not
> immediate.");
>
> Production build tends not to enable assertion for performance reason, so
> I guess we could emit an error to user for such cases. Will fix in llvm21.
>
>>
>>> +l0_%=: r0 = 0; \
>>> + exit; \
>>> +" :
>>> + : __imm_addr(map_hash_8b)
>>> + : __clobber_all);
>>> +}
>>> +
>>> SEC("socket")
>>> __description("unpriv: write into frame pointer")
>>> __failure __msg("frame pointer is read only")
>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test
2025-06-04 20:58 ` Ihor Solodrai
@ 2025-06-05 15:25 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2025-06-05 15:25 UTC (permalink / raw)
To: Ihor Solodrai, andrii; +Cc: bpf, ast, daniel, eddyz87, mykolal, kernel-team
On 6/4/25 1:58 PM, Ihor Solodrai wrote:
> On 6/4/25 1:42 PM, Yonghong Song wrote:
>>
>>
>> On 6/4/25 9:44 AM, Ihor Solodrai wrote:
>>> On 6/3/25 5:37 PM, Ihor Solodrai wrote:
>>>> Add a test for CONST_PTR_TO_MAP comparison with a non-0 constant. A
>>>> BPF program with this code must not pass verification in unpriv.
>>>>
>>>> Signed-off-by: Ihor Solodrai <isolodrai@meta.com>
>>>> ---
>>>> .../selftests/bpf/progs/verifier_unpriv.c | 17
>>>> +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/
>>>> tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> index 28200f068ce5..85b41f927272 100644
>>>> --- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> +++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
>>>> @@ -634,6 +634,23 @@ l0_%=: r0 = 0; \
>>>> : __clobber_all);
>>>> }
>>>> +SEC("socket")
>>>> +__description("unpriv: cmp map pointer with const")
>>>> +__success __failure_unpriv __msg_unpriv("R1 pointer comparison
>>>> prohibited")
>>>> +__retval(0)
>>>> +__naked void cmp_map_pointer_with_const(void)
>>>> +{
>>>> + asm volatile (" \
>>>> + r1 = 0; \
>>>> + r1 = %[map_hash_8b] ll; \
>>>> + if r1 == 0xcafefeeddeadbeef goto l0_%=; \
>>>
>>> GCC BPF caught (correctly) that this is not a valid instruction
>>> because imm is supposed to be 32bit [1]:
>>>
>>> progs/verifier_unpriv.c: Assembler messages:
>>> progs/verifier_unpriv.c:643: Error: immediate out of range,
>>> shall fit in 32 bits
>>> make: *** [Makefile:751: /tmp/work/bpf/bpf/src/tools/testing/
>>> selftests/bpf/bpf_gcc/verifier_unpriv.bpf.o] Error 1
>>>
>>> But LLVM 20 let it compile and the test passes. I wonder whether
>>> it's a bug in LLVM worth reporting?
>>>
>>> [1] https://github.com/kernel-patches/bpf/actions/runs/15430930573/
>>> job/43428666342
>>
>> This is a missed case for llvm. See:
>> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/BPF/
>> MCTargetDesc/BPFMCCodeEmitter.cpp#L82-L85
>> Basically for the following code,
>>
>> unsigned BPFMCCodeEmitter::getMachineOpValue(const MCInst &MI,
>> const MCOperand &MO,
>> SmallVectorImpl<MCFixup> &Fixups,
>> const MCSubtargetInfo
>> &STI) const {
>> if (MO.isReg())
>> return MRI.getEncodingValue(MO.getReg());
>> if (MO.isImm())
>> return static_cast<unsigned>(MO.getImm());
>>
>> For 'static_cast<unsigned>(MO.getImm())', MO.getImm() value is a s64,
>> so casting to u32 should check
>> the value range and we didn't check them, hence didn't report an error.
>
> I see. Out of curiosity I looked at llvm-objdump and indeed only lower
> 32 bits are in the binary:
>
> 0000000000000320 <cmp_map_pointer_with_const>:
> 100: b7 01 00 00 00 00 00 00 r1 = 0x0
> 101: 18 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r1 =
> 0x0 ll
> 103: 15 01 00 00 ef be ad de if r1 == -0x21524111 goto +0x0
> <l0_11>
FYI, I just submitted a fix https://github.com/llvm/llvm-project/pull/142989
which will emit the same error messages as gcc if the value cannot fit into
32bit (32bit value will do sign extension to 64bit value).
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-06-05 15:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-04 0:37 [PATCH bpf-next v2 1/3] bpf: make reg_not_null() true for CONST_PTR_TO_MAP Ihor Solodrai
2025-06-04 0:37 ` [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test Ihor Solodrai
2025-06-04 16:44 ` Ihor Solodrai
2025-06-04 20:42 ` Yonghong Song
2025-06-04 20:58 ` Ihor Solodrai
2025-06-05 15:25 ` Yonghong Song
2025-06-04 0:37 ` [PATCH bpf-next v2 3/3] selftests/bpf: add test cases with CONST_PTR_TO_MAP null checks Ihor Solodrai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).