Re: [PATCH bpf-next v1 2/3] bpf, x86: Skip the PROBE_MEM address range check under SMAP

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 8727 bytes --]

> bpf, x86: Skip the PROBE_MEM address range check under SMAP

TL;DR:
- Possible kernel oops: unprivileged (CAP_PERFMON, no CAP_BPF) programs aren't
  in kallsyms, so a NULL PROBE_MEM load under SMAP may not be fixed up
- Commit message wrongly credits arm64/riscv/loongarch; runtime cost
  unmentioned
- Stale comments (LASS, uaddress_limit)

>     bpf, x86: Skip the PROBE_MEM address range check under SMAP
>
>     The x86 JIT guards every PROBE_MEM load with a range check that keeps user
>     addresses, the guard page above TASK_SIZE_MAX and the vsyscall page away
>     from the load, because a kernel-mode fault on those addresses would oops
>     under SMAP instead of reaching the load's exception table entry. The check
>     is nine instructions and 39 bytes (32 when the offset is zero) in front of
>     a load of a few bytes. For "r7 = *(u64 *)(r0 + 2200)" on a 5-level paging
>     kernel, with VSYSCALL_ADDR and TASK_SIZE_MAX + PAGE_SIZE - VSYSCALL_ADDR as
>     the two constants:
>
>       movq    $-10485760, %r10
>       movq    %rax, %r11
>       addq    $2200, %r11
>       subq    %r10, %r11
>       movabsq $72057594048413696, %r10
>       cmpq    %r10, %r11
>       ja      load
>       xorl    %edi, %edi
>       jmp     done
>     load:
>       movq    2200(%rax), %rdi
>     done:
>
>     The previous patch made do_user_addr_fault() resolve the exception table
>     entries of BPF programs for faults on user addresses when SMAP is enabled.
>     On such kernels, emit the bare load with its exception table entry, as the
>     arm64, riscv, s390 and loongarch JITs already do. All BPF programs run with
>     SMAP active once the CPU feature is enabled, and the feature cannot change
>     after boot, so checking it at JIT time is sufficient. Kernels without SMAP,
>     including those booted with nosmap, keep the range check.
>
>     Measured with veristat over every object of the BPF selftests and over 466
>     production objects from Meta's fleet, on an x86-64 guest with SMAP, with
>     and without this series on top of bpf-next:
>
>                             programs  with PROBE_MEM  reduction per program
>                                                       mean  median    max
>       BPF selftests             3324              91  35.0%  37.5%  74.4%
>       Meta production programs  1710             228   7.0%   1.8%  69.2%
>
>     No program grows. The socket and task iterators of the selftests lose about
>     half of their code, dump_tcp6 goes from 4386 to 2124 bytes, and the
>     smallest production programs lose 60% to 69%. Loads through trusted
>     pointers and the probe_read helpers do not use PROBE_MEM, which is why most
>     programs are unaffected.
>
>     Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

The commit message says the JIT emits the bare load "as the arm64, riscv,
s390 and loongarch JITs already do".  Is that accurate for arm64, riscv and
loongarch?

Those three select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE and use the weak
bpf_arch_uaddress_limit(), which returns TASK_SIZE.  For them
bpf_do_misc_fixups() rewrites every PROBE_MEM and PROBE_MEMSX load into a
guarded sequence before the JIT sees it:

    *patch++ = BPF_MOV64_REG(BPF_REG_AX, insn->src_reg);
    if (insn->off)
            *patch++ = BPF_ALU64_IMM(BPF_ADD, BPF_REG_AX, insn->off);
    *patch++ = BPF_ALU64_IMM(BPF_RSH, BPF_REG_AX, 32);
    *patch++ = BPF_JMP_IMM(BPF_JLE, BPF_REG_AX, uaddress_limit >> 32, 2);
    *patch++ = *insn;
    *patch++ = BPF_JMP_IMM(BPF_JA, 0, 0, 1);
    *patch++ = BPF_MOV64_IMM(insn->dst_reg, 0);

So those JITs never emit a bare PROBE_MEM load for a user address.  Only
s390, which has separate address spaces, relies on the fault path in the way
the changelog describes.

With this patch, x86 with X86_FEATURE_SMAP set takes a full page fault,
is_bpf_text_address() and the exception table search for a load of NULL plus
an offset, where it used to execute a compare and a branch.  The veristat
numbers only measure code size.

Could the changelog correct the arm64/riscv/loongarch statement and say
something about the runtime cost of loads that now fault where they used to
branch?

> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 083fcd6cf15b..793e7cd5a5c4 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c

[ ... ]

> @@ -2709,6 +2710,15 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  		case BPF_LDX | BPF_PROBE_MEMSX | BPF_B:
>  		case BPF_LDX | BPF_PROBE_MEMSX | BPF_H:
>  		case BPF_LDX | BPF_PROBE_MEMSX | BPF_W:
> +			probe_mem = BPF_MODE(insn->code) == BPF_PROBE_MEM ||
> +				    BPF_MODE(insn->code) == BPF_PROBE_MEMSX;
> +			/*
> +			 * With SMAP enabled, a load from a user address faults and
> +			 * do_user_addr_fault() resolves the exception table entry of the
> +			 * program, as for an unmapped kernel address, so the address range
> +			 * check is only needed without SMAP.
> +			 */

this isn't a bug, but is this comment accurate when X86_FEATURE_LASS is
enabled?  With LASS, a supervisor load from the user half of the address
space raises a general protection fault rather than a page fault, and that is
handled on a different path:

    exc_general_protection() -> gp_try_fixup_and_notify() -> fixup_exception()

The outcome is the same, but the comment names do_user_addr_fault() as the
place where the entry is resolved.

> +			bounds_check = probe_mem && !cpu_feature_enabled(X86_FEATURE_SMAP);
>  			insn_off = insn->off;

Can this oops for a program that is not in kallsyms?  With bounds_check
false, a PROBE_MEM load of a user address such as NULL plus a field offset
depends only on the new branch in do_user_addr_fault():

    if (is_bpf_text_address(regs->ip) &&
        fixup_exception(regs, X86_TRAP_PF, error_code, address))
            return;
    ...
    page_fault_oops(regs, error_code, address);

Both is_bpf_text_address() and search_bpf_extables() only find programs that
are registered in bpf_tree, and bpf_prog_kallsyms_add() skips programs whose
loader lacks CAP_BPF:

    void bpf_prog_kallsyms_add(struct bpf_prog *fp)
    {
            if (!bpf_prog_kallsyms_candidate(fp) ||
                !bpf_token_capable(fp->aux->token, CAP_BPF))
                    return;

Such programs can still contain PROBE_MEM loads.  With
kernel.unprivileged_bpf_disabled=0, bpf_prog_load() accepts
BPF_PROG_TYPE_SOCKET_FILTER and BPF_PROG_TYPE_CGROUP_SKB without CAP_BPF.
With CAP_PERFMON, env->allow_ptr_leaks is true and bpf_sk_base_func_proto()
exposes bpf_skc_to_tcp_sock() and the other skc_to_* helpers.

skb->sk is readable as PTR_TO_SOCK_COMMON_OR_NULL, and sk_filter_trim_cap()
sets skb->sk while the filter runs.  The helper returns a plain
PTR_TO_BTF_ID, and walking a pointer field from it, for example
tp->inet_conn.icsk_ulp_ops->..., or sk_socket on an orphaned socket, gets
rewritten to BPF_PROBE_MEM by bpf_convert_ctx_accesses().  The x86 JIT then
emits an extable entry for it.

Before this patch, a NULL plus offset load in such a program was caught by
the JIT range check and dst was set to 0.  After it, on a CPU with
X86_FEATURE_SMAP the sequence would be:

    handle_page_fault() -> do_user_addr_fault() -> is_bpf_text_address()
    returns false -> page_fault_oops()

That would be an oops in softirq context (sk_filter on the receive path)
triggered by a user with CAP_PERFMON but without CAP_BPF.

arm64, riscv and loongarch are not exposed to this, because
bpf_do_misc_fixups() inserts the generic user address guard for them.

Should the range check be kept when the program will not be in kallsyms
(!bpf_token_capable(prog->aux->token, CAP_BPF)), or should every JITed
program with exception entries be registered in bpf_tree?

[ ... ]

Separately, the comment above bpf_arch_uaddress_limit() in this file:

arch/x86/net/bpf_jit_comp.c:
    /* x86-64 JIT emits its own code to filter user addresses so return 0 here */
    u64 bpf_arch_uaddress_limit(void)
    {
            return 0;
    }

is the stated reason x86 opts out of the generic verifier user address guard
in bpf_do_misc_fixups().  Is it still correct?  After this patch do_jit()
emits no filter at all when cpu_feature_enabled(X86_FEATURE_SMAP), and user
addresses are rejected only by the hardware check plus the fault handler
fixup.  Should the comment be updated, or should bpf_arch_uaddress_limit() be
made consistent with the new JIT behavior?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/37877475188