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: <ZiGEsLcInuowL25m@google.com>
Date: Thu, 18 Apr 2024 13:38:08 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: "Toke Høiland-Jørgensen" <toke@...hat.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

On 04/18, Toke Høiland-Jørgensen wrote:
> 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.

SG! Wanted to double check whether there is something I'm missing or
the existing reset logic is fine :-)

Acked-by: Stanislav Fomichev <sdf@...gle.com>