netdev - Re: [PATCH bpf] xdp: use flags field to disambiguate broadcast redirect

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: <87edb2ttdy.fsf@toke.dk>
Date: Thu, 18 Apr 2024 20:31:21 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Stanislav Fomichev <sdf@...gle.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
 <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin KaFai
 Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu
 <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>, John Fastabend
 <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Hao Luo
 <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, "David S. Miller"
 <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard
 Brouer <hawk@...nel.org>, Hangbin Liu <liuhangbin@...il.com>,
 syzbot+af9492708df9797198d6@...kaller.appspotmail.com, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf] xdp: use flags field to disambiguate broadcast
 redirect

Stanislav Fomichev <sdf@...gle.com> writes:

> On 04/18, Toke Høiland-Jørgensen wrote:
>> When redirecting a packet using XDP, the bpf_redirect_map() helper will set
>> up the redirect destination information in struct bpf_redirect_info (using
>> the __bpf_xdp_redirect_map() helper function), and the xdp_do_redirect()
>> function will read this information after the XDP program returns and pass
>> the frame on to the right redirect destination.
>> 
>> When using the BPF_F_BROADCAST flag to do multicast redirect to a whole
>> map, __bpf_xdp_redirect_map() sets the 'map' pointer in struct
>> bpf_redirect_info to point to the destination map to be broadcast. And
>> xdp_do_redirect() reacts to the value of this map pointer to decide whether
>> it's dealing with a broadcast or a single-value redirect. However, if the
>> destination map is being destroyed before xdp_do_redirect() is called, the
>> map pointer will be cleared out (by bpf_clear_redirect_map()) without
>> waiting for any XDP programs to stop running. This causes xdp_do_redirect()
>> to think that the redirect was to a single target, but the target pointer
>> is also NULL (since broadcast redirects don't have a single target), so
>> this causes a crash when a NULL pointer is passed to dev_map_enqueue().
>> 
>> To fix this, change xdp_do_redirect() to react directly to the presence of
>> the BPF_F_BROADCAST flag in the 'flags' value in struct bpf_redirect_info
>> to disambiguate between a single-target and a broadcast redirect. And only
>> read the 'map' pointer if the broadcast flag is set, aborting if that has
>> been cleared out in the meantime. This prevents the crash, while keeping
>> the atomic (cmpxchg-based) clearing of the map pointer itself, and without
>> adding any more checks in the non-broadcast fast path.
>> 
>> Fixes: e624d4ed4aa8 ("xdp: Extend xdp_redirect_map with broadcast support")
>> Reported-and-tested-by: syzbot+af9492708df9797198d6@...kaller.appspotmail.com
>> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
>> ---
>>  net/core/filter.c | 42 ++++++++++++++++++++++++++++++++----------
>>  1 file changed, 32 insertions(+), 10 deletions(-)
>> 
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 786d792ac816..8120c3dddf5e 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -4363,10 +4363,12 @@ static __always_inline int __xdp_do_redirect_frame(struct bpf_redirect_info *ri,
>>  	enum bpf_map_type map_type = ri->map_type;
>>  	void *fwd = ri->tgt_value;
>>  	u32 map_id = ri->map_id;
>> +	u32 flags = ri->flags;
>
> Any reason you copy ri->flags to the stack here? __bpf_xdp_redirect_map
> seems to be correctly resetting it for !BPF_F_BROADCAST case.

Well, we need to reset the values in xdp_do_redirect() to ensure things
are handled correctly if the next XDP program invocation returns
XDP_REDIRECT without calling bpf_redirect_map(). It's not *strictly*
necessary for the flags argument, since the other fields are reset so
that the code path that reads the flags field is never hit. But that is
not quite trivial to reason about, so I figured it was better to be
consistent with the other values here.

-Toke