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

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

[Toke Høiland-Jørgensen]

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@syzkaller.appspotmail.com
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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;
 	struct bpf_map *map;
 	int err;
 
 	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
+	ri->flags = 0;
 	ri->map_type = BPF_MAP_TYPE_UNSPEC;
 
 	if (unlikely(!xdpf)) {
@@ -4378,11 +4380,20 @@ static __always_inline int __xdp_do_redirect_frame(struct bpf_redirect_info *ri,
 	case BPF_MAP_TYPE_DEVMAP:
 		fallthrough;
 	case BPF_MAP_TYPE_DEVMAP_HASH:
-		map = READ_ONCE(ri->map);
-		if (unlikely(map)) {
+		if (unlikely(flags & BPF_F_BROADCAST)) {
+			map = READ_ONCE(ri->map);
+
+			/* The map pointer is cleared when the map is being torn
+			 * down by bpf_clear_redirect_map()
+			 */
+			if (unlikely(!map)) {
+				err = -ENOENT;
+				break;
+			}
+
 			WRITE_ONCE(ri->map, NULL);
 			err = dev_map_enqueue_multi(xdpf, dev, map,
-						    ri->flags & BPF_F_EXCLUDE_INGRESS);
+						    flags & BPF_F_EXCLUDE_INGRESS);
 		} else {
 			err = dev_map_enqueue(fwd, xdpf, dev);
 		}
@@ -4445,9 +4456,9 @@ EXPORT_SYMBOL_GPL(xdp_do_redirect_frame);
 static int xdp_do_generic_redirect_map(struct net_device *dev,
 				       struct sk_buff *skb,
 				       struct xdp_buff *xdp,
-				       struct bpf_prog *xdp_prog,
-				       void *fwd,
-				       enum bpf_map_type map_type, u32 map_id)
+				       struct bpf_prog *xdp_prog, void *fwd,
+				       enum bpf_map_type map_type, u32 map_id,
+				       u32 flags)
 {
 	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
 	struct bpf_map *map;
@@ -4457,11 +4468,20 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
 	case BPF_MAP_TYPE_DEVMAP:
 		fallthrough;
 	case BPF_MAP_TYPE_DEVMAP_HASH:
-		map = READ_ONCE(ri->map);
-		if (unlikely(map)) {
+		if (unlikely(flags & BPF_F_BROADCAST)) {
+			map = READ_ONCE(ri->map);
+
+			/* The map pointer is cleared when the map is being torn
+			 * down by bpf_clear_redirect_map()
+			 */
+			if (unlikely(!map)) {
+				err = -ENOENT;
+				break;
+			}
+
 			WRITE_ONCE(ri->map, NULL);
 			err = dev_map_redirect_multi(dev, skb, xdp_prog, map,
-						     ri->flags & BPF_F_EXCLUDE_INGRESS);
+						     flags & BPF_F_EXCLUDE_INGRESS);
 		} else {
 			err = dev_map_generic_redirect(fwd, skb, xdp_prog);
 		}
@@ -4498,9 +4518,11 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 	enum bpf_map_type map_type = ri->map_type;
 	void *fwd = ri->tgt_value;
 	u32 map_id = ri->map_id;
+	u32 flags = ri->flags;
 	int err;
 
 	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
+	ri->flags = 0;
 	ri->map_type = BPF_MAP_TYPE_UNSPEC;
 
 	if (map_type == BPF_MAP_TYPE_UNSPEC && map_id == INT_MAX) {
@@ -4520,7 +4542,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 		return 0;
 	}
 
-	return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog, fwd, map_type, map_id);
+	return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog, fwd, map_type, map_id, flags);
 err:
 	_trace_xdp_redirect_err(dev, xdp_prog, ri->tgt_index, err);
 	return err;
-- 
2.44.0

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

[Stanislav Fomichev]

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@syzkaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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.

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

[Toke Høiland-Jørgensen]

Stanislav Fomichev <sdf@google.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@syzkaller.appspotmail.com
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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

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

[Stanislav Fomichev]

On 04/18, Toke Høiland-Jørgensen wrote:
> Stanislav Fomichev <sdf@google.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@syzkaller.appspotmail.com
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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@google.com>

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

[Hangbin Liu]

On Thu, Apr 18, 2024 at 09:18:39AM +0200, 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@syzkaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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;
>  	struct bpf_map *map;
>  	int err;
>  
>  	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
> +	ri->flags = 0;
>  	ri->map_type = BPF_MAP_TYPE_UNSPEC;
>  
>  	if (unlikely(!xdpf)) {
> @@ -4378,11 +4380,20 @@ static __always_inline int __xdp_do_redirect_frame(struct bpf_redirect_info *ri,
>  	case BPF_MAP_TYPE_DEVMAP:
>  		fallthrough;
>  	case BPF_MAP_TYPE_DEVMAP_HASH:
> -		map = READ_ONCE(ri->map);
> -		if (unlikely(map)) {
> +		if (unlikely(flags & BPF_F_BROADCAST)) {
> +			map = READ_ONCE(ri->map);
> +
> +			/* The map pointer is cleared when the map is being torn
> +			 * down by bpf_clear_redirect_map()
> +			 */
> +			if (unlikely(!map)) {
> +				err = -ENOENT;
> +				break;
> +			}
> +
>  			WRITE_ONCE(ri->map, NULL);
>  			err = dev_map_enqueue_multi(xdpf, dev, map,
> -						    ri->flags & BPF_F_EXCLUDE_INGRESS);
> +						    flags & BPF_F_EXCLUDE_INGRESS);
>  		} else {
>  			err = dev_map_enqueue(fwd, xdpf, dev);
>  		}
> @@ -4445,9 +4456,9 @@ EXPORT_SYMBOL_GPL(xdp_do_redirect_frame);
>  static int xdp_do_generic_redirect_map(struct net_device *dev,
>  				       struct sk_buff *skb,
>  				       struct xdp_buff *xdp,
> -				       struct bpf_prog *xdp_prog,
> -				       void *fwd,
> -				       enum bpf_map_type map_type, u32 map_id)
> +				       struct bpf_prog *xdp_prog, void *fwd,
> +				       enum bpf_map_type map_type, u32 map_id,
> +				       u32 flags)
>  {
>  	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>  	struct bpf_map *map;
> @@ -4457,11 +4468,20 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
>  	case BPF_MAP_TYPE_DEVMAP:
>  		fallthrough;
>  	case BPF_MAP_TYPE_DEVMAP_HASH:
> -		map = READ_ONCE(ri->map);
> -		if (unlikely(map)) {
> +		if (unlikely(flags & BPF_F_BROADCAST)) {
> +			map = READ_ONCE(ri->map);
> +
> +			/* The map pointer is cleared when the map is being torn
> +			 * down by bpf_clear_redirect_map()
> +			 */
> +			if (unlikely(!map)) {
> +				err = -ENOENT;
> +				break;
> +			}
> +
>  			WRITE_ONCE(ri->map, NULL);
>  			err = dev_map_redirect_multi(dev, skb, xdp_prog, map,
> -						     ri->flags & BPF_F_EXCLUDE_INGRESS);
> +						     flags & BPF_F_EXCLUDE_INGRESS);
>  		} else {
>  			err = dev_map_generic_redirect(fwd, skb, xdp_prog);
>  		}
> @@ -4498,9 +4518,11 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>  	enum bpf_map_type map_type = ri->map_type;
>  	void *fwd = ri->tgt_value;
>  	u32 map_id = ri->map_id;
> +	u32 flags = ri->flags;
>  	int err;
>  
>  	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
> +	ri->flags = 0;
>  	ri->map_type = BPF_MAP_TYPE_UNSPEC;
>  
>  	if (map_type == BPF_MAP_TYPE_UNSPEC && map_id == INT_MAX) {
> @@ -4520,7 +4542,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>  		return 0;
>  	}
>  
> -	return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog, fwd, map_type, map_id);
> +	return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog, fwd, map_type, map_id, flags);
>  err:
>  	_trace_xdp_redirect_err(dev, xdp_prog, ri->tgt_index, err);
>  	return err;
> -- 
> 2.44.0
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

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

[Jesper Dangaard Brouer]

On 18/04/2024 09.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@syzkaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen<toke@redhat.com>
> ---
>   net/core/filter.c | 42 ++++++++++++++++++++++++++++++++----------
>   1 file changed, 32 insertions(+), 10 deletions(-)

Thanks for finding root-cause and fixing this!

Acked-by: Jesper Dangaard Brouer <hawk@kernel.org>

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

[Martin KaFai Lau]

On 4/18/24 12:18 AM, 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@syzkaller.appspotmail.com
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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;
>   	struct bpf_map *map;
>   	int err;
>   
>   	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
> +	ri->flags = 0;
>   	ri->map_type = BPF_MAP_TYPE_UNSPEC;
>   
>   	if (unlikely(!xdpf)) {
> @@ -4378,11 +4380,20 @@ static __always_inline int __xdp_do_redirect_frame(struct bpf_redirect_info *ri,
>   	case BPF_MAP_TYPE_DEVMAP:
>   		fallthrough;
>   	case BPF_MAP_TYPE_DEVMAP_HASH:
> -		map = READ_ONCE(ri->map);
> -		if (unlikely(map)) {
> +		if (unlikely(flags & BPF_F_BROADCAST)) {
> +			map = READ_ONCE(ri->map);
> +
> +			/* The map pointer is cleared when the map is being torn
> +			 * down by bpf_clear_redirect_map()

Thanks for the details explanation in the commit message. All make sense.

It could be a dumb question.

 From reading the "waits for...NAPI being the relevant context here..." comment 
in dev_map_free(), I wonder if moving synchronize_rcu() before 
bpf_clear_redirect_map() would also work? Actually, does it need to call 
bpf_clear_redirect_map(). The on-going xdp_do_redirect() should be the last one 
using the map in ri->map anyway and no xdp prog can set it again to ri->map.

> +			 */
> +			if (unlikely(!map)) {
> +				err = -ENOENT;
> +				break;
> +			}
> +
>   			WRITE_ONCE(ri->map, NULL);
>   			err = dev_map_enqueue_multi(xdpf, dev, map,
> -						    ri->flags & BPF_F_EXCLUDE_INGRESS);
> +						    flags & BPF_F_EXCLUDE_INGRESS);
>   		} else {
>   			err = dev_map_enqueue(fwd, xdpf, dev);
>   		}

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

[Toke Høiland-Jørgensen]

Martin KaFai Lau <martin.lau@linux.dev> writes:

> On 4/18/24 12:18 AM, 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@syzkaller.appspotmail.com
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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;
>>   	struct bpf_map *map;
>>   	int err;
>>   
>>   	ri->map_id = 0; /* Valid map id idr range: [1,INT_MAX[ */
>> +	ri->flags = 0;
>>   	ri->map_type = BPF_MAP_TYPE_UNSPEC;
>>   
>>   	if (unlikely(!xdpf)) {
>> @@ -4378,11 +4380,20 @@ static __always_inline int __xdp_do_redirect_frame(struct bpf_redirect_info *ri,
>>   	case BPF_MAP_TYPE_DEVMAP:
>>   		fallthrough;
>>   	case BPF_MAP_TYPE_DEVMAP_HASH:
>> -		map = READ_ONCE(ri->map);
>> -		if (unlikely(map)) {
>> +		if (unlikely(flags & BPF_F_BROADCAST)) {
>> +			map = READ_ONCE(ri->map);
>> +
>> +			/* The map pointer is cleared when the map is being torn
>> +			 * down by bpf_clear_redirect_map()
>
> Thanks for the details explanation in the commit message. All make sense.

Great!

> It could be a dumb question.
>
>  From reading the "waits for...NAPI being the relevant context here..." comment 
> in dev_map_free(), I wonder if moving synchronize_rcu() before 
> bpf_clear_redirect_map() would also work? Actually, does it need to call 
> bpf_clear_redirect_map(). The on-going xdp_do_redirect() should be the last one 
> using the map in ri->map anyway and no xdp prog can set it again to
>  ri->map.

I think we do need to retain the current behaviour, because of the
decoupling between the helper and the return code. Otherwise, you could
have a program that calls the bpf_redirect_map() helper, but returns a
different value (say, XDP_DROP). In this case, the map pointer will
stick around in struct bpf_redirect_info, and if a subsequent XDP
program then returns XDP_REDIRECT (*without* calling
bpf_redirect_map()), it will use the stale pointer value and cause a
UAF.

-Toke

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Thu, 18 Apr 2024 09:18:39 +0200 you 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().
> 
> [...]

Here is the summary with links:
  - [bpf] xdp: use flags field to disambiguate broadcast redirect
    https://git.kernel.org/bpf/bpf/c/5bcf0dcbf906

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Martin KaFai Lau]

On 4/20/24 3:24 AM, Toke Høiland-Jørgensen wrote:
>>   From reading the "waits for...NAPI being the relevant context here..." comment
>> in dev_map_free(), I wonder if moving synchronize_rcu() before
>> bpf_clear_redirect_map() would also work? Actually, does it need to call
>> bpf_clear_redirect_map(). The on-going xdp_do_redirect() should be the last one
>> using the map in ri->map anyway and no xdp prog can set it again to
>>   ri->map.
> I think we do need to retain the current behaviour, because of the
> decoupling between the helper and the return code. Otherwise, you could

Forgot there could be a disconnect here.

Applied. Thanks.

> have a program that calls the bpf_redirect_map() helper, but returns a
> different value (say, XDP_DROP). In this case, the map pointer will
> stick around in struct bpf_redirect_info, and if a subsequent XDP
> program then returns XDP_REDIRECT (*without*  calling
> bpf_redirect_map()), it will use the stale pointer value and cause a