netdev - Re: [RFC] Use after free in BPF/ XDP during XDP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874izshrvs.fsf@toke.dk>
Date: Mon, 17 Mar 2025 11:29:43 +0100
From: Toke Høiland-Jørgensen <toke@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, Ricardo Cañuelo Navarro
 <rcn@...lia.com>, Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko
 <andrii@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, "David S.
 Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Jesper
 Dangaard Brouer <hawk@...nel.org>, John Fastabend
 <john.fastabend@...il.com>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC] Use after free in BPF/ XDP during XDP_REDIRECT

Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:

> On 2025-03-14 17:03:35 [+0100], Toke Høiland-Jørgensen wrote:
>> > While at it, is there anything that ensures that only bpf_prog_run_xdp()
>> > can invoke the map_redirect callback? Mainline only assigns the task
>> > pointer in NAPI callback so any usage outside of bpf_prog_run_xdp() will
>> > lead to a segfault and I haven't seen a report yet so…
>> 
>> Yes, the verifier restricts which program types can call the
>> map_redirect helper.
>
> Okay. So checks for the BPF_PROG_TYPE_XDP type for the map_redirect and
> that is the only one setting it. Okay. Now I remember Alexei mentioning
> something…

Yeah, there's basically a mapping between BPF program types and the
available helpers. For XDP this is in xdp_func_proto() in net/core/filter.c.

>> > --- a/include/net/xdp.h
>> > +++ b/include/net/xdp.h
>> > @@ -486,7 +486,12 @@ static __always_inline u32 bpf_prog_run_xdp(const struct bpf_prog *prog,
>> >  	 * under local_bh_disable(), which provides the needed RCU protection
>> >  	 * for accessing map entries.
>> >  	 */
>> > -	u32 act = __bpf_prog_run(prog, xdp, BPF_DISPATCHER_FUNC(xdp));
>> > +	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>> > +	u32 act;
>> > +
>> 
>> Add an if here like
>> 
>> if (ri->map_id | ri->map_type) { /* single | to make it a single branch */
>> 
>> > +	ri->map_id = INT_MAX;
>> > +	ri->map_type = BPF_MAP_TYPE_UNSPEC;
>> 
>> }
>> 
>> Also, ri->map_id should be set to 0, not INT_MAX.
>
> The or variant does
>
> |         add %gs:this_cpu_off(%rip), %rax        # this_cpu_off, tcp_ptr__
> |         movl    32(%rax), %edx  # _51->map_id, _51->map_id
> |         orl     36(%rax), %edx  # _51->map_type, tmp311
> |         je      .L1546  #,
> |         movq    $0, 32(%rax)    #, MEM <vector(2) unsigned int> [(unsigned int *)_51 + 32B]
> | .L1546:
>
> while the || does
>
> |         add %gs:this_cpu_off(%rip), %rax        # this_cpu_off, tcp_ptr__
> |         cmpq    $0, 32(%rax)    #, *_51
> |         je      .L1546  #,
> |         movq    $0, 32(%rax)    #, MEM <vector(2) unsigned int> [(unsigned int *)_51 + 32B]
> | .L1546:
>
> gcc isn't bad at optimizing here ;)

Ohh, neat! Didn't consider that this is two U32s, so they can be loaded
in one go. That's what I get from trying to second-guess the compiler, I
suppose :)

Let's just go with the obvious one (||) instead of the OR thing, then.

> This is the or version as asked for. I don't mind doing any of the both.
> I everyone agrees then I would send it to Greg.

Sure, with the above, feel free to add my:

Acked-by: Toke Høiland-Jørgensen <toke@...nel.org>