Re: [PATCH bpf-next v2 2/3] selftests/bpf: add cmp_map_pointer_with_const test

[Yonghong Song <yonghong.song@linux.dev>]

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")
>
>