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

[Ihor Solodrai <ihor.solodrai@linux.dev>]

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