All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] bpf, x86: Fix per-CPU address resolution into an extended register
@ 2026-08-14 16:55 Vineet Gupta
  2026-08-14 19:13 ` Eduard Zingerman
  0 siblings, 1 reply; 3+ messages in thread
From: Vineet Gupta @ 2026-08-14 16:55 UTC (permalink / raw)
  To: bpf; +Cc: ast, daniel, andrii, x86, stable, Vineet Gupta

The destination of the per-CPU address MOV is encoded in ModRM.reg,
which is extended by REX.R, but the REX prefix is built with
add_1mod(), which sets REX.B. REX.B extends ModRM.rm and SIB.base, and
this instruction addresses memory as disp32 with no base, so the bit
has no effect at all and the high register bit is simply lost.

Every is_ereg() destination therefore resolves to the wrong register,
picking whichever one shares the low three bits:

  R5 -> RAX    R7 -> RBP    R8 -> RSI    R9 -> RDI

With BPF_REG_5, whose reg2hex is 0, the emitted

  65 49 03 04 25 <off>	add %gs:<off>,%rax

adds the per-CPU offset to RAX rather than R8. The destination keeps
the unadjusted address and RAX is clobbered, so the program goes on to
dereference a pointer that was never made per-CPU:

  BUG: unable to handle page fault for address: 0000607e386a8894
  RIP: bpf_prog_707837aafd2aa9ae_update_percpu_data+0x93/0xc9
  Call Trace:
   __bpf_prog_test_run_raw_tp+0x2dc/0x7d0
   __flush_smp_call_function_queue+0x1e9/0xc80
  Kernel panic - not syncing: Fatal exception in interrupt

R5 is the mildest of the four, aliasing a scratch register and faulting
at the store. R7 aliases RBP and would corrupt the frame pointer, R8
and R9 alias the argument registers.

Use add_2mod() so the register goes through REX.R, matching how
add_2reg() places it in ModRM.reg and how emit_priv_frame_ptr()
hardcodes 0x4c for the same instruction with R9. Encodings for the
non-extended registers are unchanged.

Problem showed up when trying to resurrect BPF_GCC CI (selftests built
with BPF_GCC).

This has gone unnoticed because clang reloads the address into R1
before each per-CPU access, so the destination is never an extended
register. GCC keeps several per-CPU addresses live at once, and
test_progs-bpf_gcc panics the kernel in global_percpu_data/init, where
the address of a .percpu variable ends up in R5.

Fixes: 7bdbf7446305 ("bpf: add special internal-only MOV instruction to resolve per-CPU addrs")
Cc: stable@vger.kernel.org
Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
---
 arch/x86/net/bpf_jit_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index d920772af7d5..1a9fb530adc3 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1935,7 +1935,7 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
 				EMIT_mov(dst_reg, src_reg);
 #ifdef CONFIG_SMP
 				/* add <dst>, gs:[<off>] */
-				EMIT2(0x65, add_1mod(0x48, dst_reg));
+				EMIT2(0x65, add_2mod(0x48, 0, dst_reg));
 				EMIT3(0x03, add_2reg(0x04, 0, dst_reg), 0x25);
 				EMIT((u32)(unsigned long)&this_cpu_off, 4);
 #endif
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] bpf, x86: Fix per-CPU address resolution into an extended register
  2026-08-14 16:55 [PATCH] bpf, x86: Fix per-CPU address resolution into an extended register Vineet Gupta
@ 2026-08-14 19:13 ` Eduard Zingerman
  2026-08-14 19:45   ` Vineet Gupta
  0 siblings, 1 reply; 3+ messages in thread
From: Eduard Zingerman @ 2026-08-14 19:13 UTC (permalink / raw)
  To: Vineet Gupta, bpf; +Cc: ast, daniel, andrii, x86, stable

On Fri, 2026-08-14 at 09:55 -0700, Vineet Gupta wrote:
> The destination of the per-CPU address MOV is encoded in ModRM.reg,
> which is extended by REX.R, but the REX prefix is built with
> add_1mod(), which sets REX.B. REX.B extends ModRM.rm and SIB.base, and
> this instruction addresses memory as disp32 with no base, so the bit
> has no effect at all and the high register bit is simply lost.
> 
> Every is_ereg() destination therefore resolves to the wrong register,
> picking whichever one shares the low three bits:
> 
>   R5 -> RAX    R7 -> RBP    R8 -> RSI    R9 -> RDI
> 
> With BPF_REG_5, whose reg2hex is 0, the emitted
> 
>   65 49 03 04 25 <off>	add %gs:<off>,%rax
> 
> adds the per-CPU offset to RAX rather than R8. The destination keeps
> the unadjusted address and RAX is clobbered, so the program goes on to
> dereference a pointer that was never made per-CPU:
> 
>   BUG: unable to handle page fault for address: 0000607e386a8894
>   RIP: bpf_prog_707837aafd2aa9ae_update_percpu_data+0x93/0xc9
>   Call Trace:
>    __bpf_prog_test_run_raw_tp+0x2dc/0x7d0
>    __flush_smp_call_function_queue+0x1e9/0xc80
>   Kernel panic - not syncing: Fatal exception in interrupt
> 
> R5 is the mildest of the four, aliasing a scratch register and faulting
> at the store. R7 aliases RBP and would corrupt the frame pointer, R8
> and R9 alias the argument registers.
> 
> Use add_2mod() so the register goes through REX.R, matching how
> add_2reg() places it in ModRM.reg and how emit_priv_frame_ptr()
> hardcodes 0x4c for the same instruction with R9. Encodings for the
> non-extended registers are unchanged.
> 
> Problem showed up when trying to resurrect BPF_GCC CI (selftests built
> with BPF_GCC).
> 
> This has gone unnoticed because clang reloads the address into R1
> before each per-CPU access, so the destination is never an extended
> register. GCC keeps several per-CPU addresses live at once, and
> test_progs-bpf_gcc panics the kernel in global_percpu_data/init, where
> the address of a .percpu variable ends up in R5.
> 
> Fixes: 7bdbf7446305 ("bpf: add special internal-only MOV instruction to resolve per-CPU addrs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
> ---
>  arch/x86/net/bpf_jit_comp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index d920772af7d5..1a9fb530adc3 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1935,7 +1935,7 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>  				EMIT_mov(dst_reg, src_reg);
>  #ifdef CONFIG_SMP
>  				/* add <dst>, gs:[<off>] */
> -				EMIT2(0x65, add_1mod(0x48, dst_reg));
> +				EMIT2(0x65, add_2mod(0x48, 0, dst_reg));

Ok, so the argument is that because there is a SIB byte in this
instruction, and the instruction itself is `REX.W + 03 /r | ADD r64, r/m64`,
operand #1 ModRM:reg, operand #2 ModRM:r/m, the encoding should adjust
REX.R to extend operand #1, not REX.B as it does now.

I think this is correct. Could you please respin adding an inline
assembly test with __xlated() matcher, to verify that jit operates
correctly?

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

>  				EMIT3(0x03, add_2reg(0x04, 0, dst_reg), 0x25);
>  				EMIT((u32)(unsigned long)&this_cpu_off, 4);
>  #endif

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] bpf, x86: Fix per-CPU address resolution into an extended register
  2026-08-14 19:13 ` Eduard Zingerman
@ 2026-08-14 19:45   ` Vineet Gupta
  0 siblings, 0 replies; 3+ messages in thread
From: Vineet Gupta @ 2026-08-14 19:45 UTC (permalink / raw)
  To: Eduard Zingerman, bpf; +Cc: ast, daniel, andrii, x86, stable

On 8/14/26 12:13 PM, Eduard Zingerman wrote:
> On Fri, 2026-08-14 at 09:55 -0700, Vineet Gupta wrote:
>> The destination of the per-CPU address MOV is encoded in ModRM.reg,
>> which is extended by REX.R, but the REX prefix is built with
>> add_1mod(), which sets REX.B. REX.B extends ModRM.rm and SIB.base, and
>> this instruction addresses memory as disp32 with no base, so the bit
>> has no effect at all and the high register bit is simply lost.
>>
>> Every is_ereg() destination therefore resolves to the wrong register,
>> picking whichever one shares the low three bits:
>>
>>    R5 -> RAX    R7 -> RBP    R8 -> RSI    R9 -> RDI
>>
>> With BPF_REG_5, whose reg2hex is 0, the emitted
>>
>>    65 49 03 04 25 <off>	add %gs:<off>,%rax
>>
>> adds the per-CPU offset to RAX rather than R8. The destination keeps
>> the unadjusted address and RAX is clobbered, so the program goes on to
>> dereference a pointer that was never made per-CPU:
>>
>>    BUG: unable to handle page fault for address: 0000607e386a8894
>>    RIP: bpf_prog_707837aafd2aa9ae_update_percpu_data+0x93/0xc9
>>    Call Trace:
>>     __bpf_prog_test_run_raw_tp+0x2dc/0x7d0
>>     __flush_smp_call_function_queue+0x1e9/0xc80
>>    Kernel panic - not syncing: Fatal exception in interrupt
>>
>> R5 is the mildest of the four, aliasing a scratch register and faulting
>> at the store. R7 aliases RBP and would corrupt the frame pointer, R8
>> and R9 alias the argument registers.
>>
>> Use add_2mod() so the register goes through REX.R, matching how
>> add_2reg() places it in ModRM.reg and how emit_priv_frame_ptr()
>> hardcodes 0x4c for the same instruction with R9. Encodings for the
>> non-extended registers are unchanged.
>>
>> Problem showed up when trying to resurrect BPF_GCC CI (selftests built
>> with BPF_GCC).
>>
>> This has gone unnoticed because clang reloads the address into R1
>> before each per-CPU access, so the destination is never an extended
>> register. GCC keeps several per-CPU addresses live at once, and
>> test_progs-bpf_gcc panics the kernel in global_percpu_data/init, where
>> the address of a .percpu variable ends up in R5.
>>
>> Fixes: 7bdbf7446305 ("bpf: add special internal-only MOV instruction to resolve per-CPU addrs")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
>> ---
>>   arch/x86/net/bpf_jit_comp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index d920772af7d5..1a9fb530adc3 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -1935,7 +1935,7 @@ static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *
>>   				EMIT_mov(dst_reg, src_reg);
>>   #ifdef CONFIG_SMP
>>   				/* add <dst>, gs:[<off>] */
>> -				EMIT2(0x65, add_1mod(0x48, dst_reg));
>> +				EMIT2(0x65, add_2mod(0x48, 0, dst_reg));
> Ok, so the argument is that because there is a SIB byte in this
> instruction, and the instruction itself is `REX.W + 03 /r | ADD r64, r/m64`,
> operand #1 ModRM:reg, operand #2 ModRM:r/m, the encoding should adjust
> REX.R to extend operand #1, not REX.B as it does now.
>
> I think this is correct. Could you please respin adding an inline
> assembly test with __xlated() matcher, to verify that jit operates
> correctly?

xlated or jited ?

> Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

Thanks for the quick review !

-Vineet

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-08-14 19:45 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-14 16:55 [PATCH] bpf, x86: Fix per-CPU address resolution into an extended register Vineet Gupta
2026-08-14 19:13 ` Eduard Zingerman
2026-08-14 19:45   ` Vineet Gupta

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.