[bpf,v2] xsk: fix xsk_diag use-after-free error during socket cleanup

Commit Message

Magnus Karlsson Aug. 31, 2023, 10:01 a.m. UTC

From: Magnus Karlsson <[e-mail]>

Fix a use-after-free error that is possible if the xsk_diag interface
is used after the socket has been unbound from the device. This can
happen either due to the socket being closed or the device
disappearing. In the early days of AF_XDP, the way we tested that a
socket was not bound to a device was to simply check if the netdevice
pointer in the xsk socket structure was NULL. Later, a better system
was introduced by having an explicit state variable in the xsk socket
struct. For example, the state of a socket that is on the way to being
closed and has been unbound from the device is XSK_UNBOUND.

The commit in the Fixes tag below deleted the old way of signalling
that a socket is unbound, setting dev to NULL. This in the belief that
all code using the old way had been exterminated. That was
unfortunately not true as the xsk diagnostics code was still using the
old way and thus does not work as intended when a socket is going
down. Fix this by introducing a test against the state variable. If
the socket is in the state XSK_UNBOUND, simply abort the diagnostic's
netlink operation.

Fixes: 18b1ab7aa76b ("xsk: Fix race at socket teardown")
Reported-and-tested-by: [e-mail]
Signed-off-by: Magnus Karlsson <[e-mail]>
---
v1 -> v2:
  * Added READ_ONCE for the state variable [Magnus]
  * Improved commit message [Maciej]

 net/xdp/xsk_diag.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 7d35eb1a184a3f0759ad9e9cde4669b5c55b2063
--
2.42.0

Comments

Fijalkowski, Maciej Aug. 31, 2023, 10:39 a.m. UTC | #1

On Thu, Aug 31, 2023 at 12:01:17PM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <[e-mail]>
> 
> Fix a use-after-free error that is possible if the xsk_diag interface
> is used after the socket has been unbound from the device. This can
> happen either due to the socket being closed or the device
> disappearing. In the early days of AF_XDP, the way we tested that a
> socket was not bound to a device was to simply check if the netdevice
> pointer in the xsk socket structure was NULL. Later, a better system
> was introduced by having an explicit state variable in the xsk socket
> struct. For example, the state of a socket that is on the way to being
> closed and has been unbound from the device is XSK_UNBOUND.
> 
> The commit in the Fixes tag below deleted the old way of signalling
> that a socket is unbound, setting dev to NULL. This in the belief that
> all code using the old way had been exterminated. That was
> unfortunately not true as the xsk diagnostics code was still using the
> old way and thus does not work as intended when a socket is going
> down. Fix this by introducing a test against the state variable. If
> the socket is in the state XSK_UNBOUND, simply abort the diagnostic's
> netlink operation.
> 
> Fixes: 18b1ab7aa76b ("xsk: Fix race at socket teardown")
> Reported-and-tested-by: [e-mail]
> Signed-off-by: Magnus Karlsson <[e-mail]>

Reviewed-by: Maciej Fijalkowski <[e-mail]>

> ---
> v1 -> v2:
>   * Added READ_ONCE for the state variable [Magnus]
>   * Improved commit message [Maciej]
> 
>  net/xdp/xsk_diag.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> index c014217f5fa7..22b36c8143cf 100644
> --- a/net/xdp/xsk_diag.c
> +++ b/net/xdp/xsk_diag.c
> @@ -111,6 +111,9 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
>  	sock_diag_save_cookie(sk, msg->xdiag_cookie);
> 
>  	mutex_lock(&xs->mutex);
> +	if (READ_ONCE(xs->state) == XSK_UNBOUND)
> +		goto out_nlmsg_trim;
> +
>  	if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
>  		goto out_nlmsg_trim;
> 
> 
> base-commit: 7d35eb1a184a3f0759ad9e9cde4669b5c55b2063
> --
> 2.42.0

Fijalkowski, Maciej Aug. 31, 2023, 10:55 a.m. UTC | #2

On Thu, Aug 31, 2023 at 12:39:21PM +0200, Maciej Fijalkowski wrote:
> On Thu, Aug 31, 2023 at 12:01:17PM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <[e-mail]>
> > 
> > Fix a use-after-free error that is possible if the xsk_diag interface
> > is used after the socket has been unbound from the device. This can
> > happen either due to the socket being closed or the device
> > disappearing. In the early days of AF_XDP, the way we tested that a
> > socket was not bound to a device was to simply check if the netdevice
> > pointer in the xsk socket structure was NULL. Later, a better system
> > was introduced by having an explicit state variable in the xsk socket
> > struct. For example, the state of a socket that is on the way to being
> > closed and has been unbound from the device is XSK_UNBOUND.
> > 
> > The commit in the Fixes tag below deleted the old way of signalling
> > that a socket is unbound, setting dev to NULL. This in the belief that
> > all code using the old way had been exterminated. That was
> > unfortunately not true as the xsk diagnostics code was still using the
> > old way and thus does not work as intended when a socket is going
> > down. Fix this by introducing a test against the state variable. If
> > the socket is in the state XSK_UNBOUND, simply abort the diagnostic's
> > netlink operation.
> > 
> > Fixes: 18b1ab7aa76b ("xsk: Fix race at socket teardown")
> > Reported-and-tested-by: [e-mail]
> > Signed-off-by: Magnus Karlsson <[e-mail]>
> 
> Reviewed-by: Maciej Fijalkowski <[e-mail]>

FWIW also tested that issue is no longer triggered on my local system:
Tested-by: Maciej Fijalkowski <[e-mail]>


> 
> > ---
> > v1 -> v2:
> >   * Added READ_ONCE for the state variable [Magnus]
> >   * Improved commit message [Maciej]
> > 
> >  net/xdp/xsk_diag.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> > index c014217f5fa7..22b36c8143cf 100644
> > --- a/net/xdp/xsk_diag.c
> > +++ b/net/xdp/xsk_diag.c
> > @@ -111,6 +111,9 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
> >  	sock_diag_save_cookie(sk, msg->xdiag_cookie);
> > 
> >  	mutex_lock(&xs->mutex);
> > +	if (READ_ONCE(xs->state) == XSK_UNBOUND)
> > +		goto out_nlmsg_trim;
> > +
> >  	if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
> >  		goto out_nlmsg_trim;
> > 
> > 
> > base-commit: 7d35eb1a184a3f0759ad9e9cde4669b5c55b2063
> > --
> > 2.42.0

[e-mail] Aug. 31, 2023, 11:30 a.m. UTC | #3

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <[e-mail]>:

On Thu, 31 Aug 2023 12:01:17 +0200 you wrote:
> From: Magnus Karlsson <[e-mail]>
> 
> Fix a use-after-free error that is possible if the xsk_diag interface
> is used after the socket has been unbound from the device. This can
> happen either due to the socket being closed or the device
> disappearing. In the early days of AF_XDP, the way we tested that a
> socket was not bound to a device was to simply check if the netdevice
> pointer in the xsk socket structure was NULL. Later, a better system
> was introduced by having an explicit state variable in the xsk socket
> struct. For example, the state of a socket that is on the way to being
> closed and has been unbound from the device is XSK_UNBOUND.
> 
> [...]

Here is the summary with links:
  - [bpf,v2] xsk: fix xsk_diag use-after-free error during socket cleanup
    https://git.kernel.org/bpf/bpf/c/3e019d8a05a3

You are awesome, thank you!

Patch

diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
index c014217f5fa7..22b36c8143cf 100644
--- a/net/xdp/xsk_diag.c
+++ b/net/xdp/xsk_diag.c
@@ -111,6 +111,9 @@  static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
 	sock_diag_save_cookie(sk, msg->xdiag_cookie);

 	mutex_lock(&xs->mutex);
+	if (READ_ONCE(xs->state) == XSK_UNBOUND)
+		goto out_nlmsg_trim;
+
 	if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
 		goto out_nlmsg_trim;