[PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete

[PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete

[Magnus Karlsson]

This small series fixes two bugs in xsk_socket__delete. Details can be
found in the individual commit messages, but a brief summary follows:

Patch 1: fix null pointer dereference in xsk_socket__delete
Patch 2: fix possible use after free in xsk_socket__delete

This patch has been applied against commit 7a078d2d1880 ("libbpf, hashmap: Fix undefined behavior in hash_bits")

Thanks: Magnus

Magnus Karlsson (2):
  libbpf: fix null dereference in xsk_socket__delete
  libbpf: fix possible use after free in xsk_socket__delete

 tools/lib/bpf/xsk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--
2.7.4

[PATCH bpf 1/2] libbpf: fix null dereference in xsk_socket__delete

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix a possible null pointer dereference in xsk_socket__delete that
will occur if a null pointer is fed into the function.

Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/lib/bpf/xsk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index e3c98c0..504b7a8 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -891,13 +891,14 @@ int xsk_umem__delete(struct xsk_umem *umem)
 void xsk_socket__delete(struct xsk_socket *xsk)
 {
 	size_t desc_sz = sizeof(struct xdp_desc);
-	struct xsk_ctx *ctx = xsk->ctx;
 	struct xdp_mmap_offsets off;
+	struct xsk_ctx *ctx;
 	int err;
 
 	if (!xsk)
 		return;
 
+	ctx = xsk->ctx;
 	if (ctx->prog_fd != -1) {
 		xsk_delete_bpf_maps(xsk);
 		close(ctx->prog_fd);
-- 
2.7.4

[PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete

[Magnus Karlsson]

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix a possible use after free in xsk_socket__delete that will happen
if xsk_put_ctx() frees the ctx. To fix, save the umem reference taken
from the context and just use that instead.

Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 tools/lib/bpf/xsk.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 504b7a8..9bc537d 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -892,6 +892,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 {
 	size_t desc_sz = sizeof(struct xdp_desc);
 	struct xdp_mmap_offsets off;
+	struct xsk_umem *umem;
 	struct xsk_ctx *ctx;
 	int err;
 
@@ -899,6 +900,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 		return;
 
 	ctx = xsk->ctx;
+	umem = ctx->umem;
 	if (ctx->prog_fd != -1) {
 		xsk_delete_bpf_maps(xsk);
 		close(ctx->prog_fd);
@@ -918,11 +920,11 @@ void xsk_socket__delete(struct xsk_socket *xsk)
 
 	xsk_put_ctx(ctx);
 
-	ctx->umem->refcount--;
+	umem->refcount--;
 	/* Do not close an fd that also has an associated umem connected
 	 * to it.
 	 */
-	if (xsk->fd != ctx->umem->fd)
+	if (xsk->fd != umem->fd)
 		close(xsk->fd);
 	free(xsk);
 }
-- 
2.7.4

Re: [PATCH bpf 1/2] libbpf: fix null dereference in xsk_socket__delete

[Andrii Nakryiko]

On Tue, Nov 3, 2020 at 1:41 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> From: Magnus Karlsson <magnus.karlsson@intel.com>
>
> Fix a possible null pointer dereference in xsk_socket__delete that
> will occur if a null pointer is fed into the function.
>
> Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/lib/bpf/xsk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index e3c98c0..504b7a8 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -891,13 +891,14 @@ int xsk_umem__delete(struct xsk_umem *umem)
>  void xsk_socket__delete(struct xsk_socket *xsk)
>  {
>         size_t desc_sz = sizeof(struct xdp_desc);
> -       struct xsk_ctx *ctx = xsk->ctx;
>         struct xdp_mmap_offsets off;
> +       struct xsk_ctx *ctx;
>         int err;
>
>         if (!xsk)
>                 return;
>
> +       ctx = xsk->ctx;
>         if (ctx->prog_fd != -1) {
>                 xsk_delete_bpf_maps(xsk);
>                 close(ctx->prog_fd);
> --
> 2.7.4
>

Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete

[Andrii Nakryiko]

On Tue, Nov 3, 2020 at 1:42 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> From: Magnus Karlsson <magnus.karlsson@intel.com>
>
> Fix a possible use after free in xsk_socket__delete that will happen
> if xsk_put_ctx() frees the ctx. To fix, save the umem reference taken
> from the context and just use that instead.
>
> Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
>  tools/lib/bpf/xsk.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 504b7a8..9bc537d 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -892,6 +892,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>  {
>         size_t desc_sz = sizeof(struct xdp_desc);
>         struct xdp_mmap_offsets off;
> +       struct xsk_umem *umem;
>         struct xsk_ctx *ctx;
>         int err;
>
> @@ -899,6 +900,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>                 return;
>
>         ctx = xsk->ctx;
> +       umem = ctx->umem;
>         if (ctx->prog_fd != -1) {
>                 xsk_delete_bpf_maps(xsk);
>                 close(ctx->prog_fd);
> @@ -918,11 +920,11 @@ void xsk_socket__delete(struct xsk_socket *xsk)
>
>         xsk_put_ctx(ctx);
>
> -       ctx->umem->refcount--;
> +       umem->refcount--;

if you moved ctx->umem->refcount--; to before xdk_put_ctx(ctx), would
that also work?

>         /* Do not close an fd that also has an associated umem connected
>          * to it.
>          */
> -       if (xsk->fd != ctx->umem->fd)
> +       if (xsk->fd != umem->fd)
>                 close(xsk->fd);
>         free(xsk);
>  }
> --
> 2.7.4
>

Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete

[Magnus Karlsson]

On Tue, Nov 3, 2020 at 8:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 1:42 AM Magnus Karlsson
> <magnus.karlsson@gmail.com> wrote:
> >
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Fix a possible use after free in xsk_socket__delete that will happen
> > if xsk_put_ctx() frees the ctx. To fix, save the umem reference taken
> > from the context and just use that instead.
> >
> > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> >  tools/lib/bpf/xsk.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index 504b7a8..9bc537d 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -892,6 +892,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> >  {
> >         size_t desc_sz = sizeof(struct xdp_desc);
> >         struct xdp_mmap_offsets off;
> > +       struct xsk_umem *umem;
> >         struct xsk_ctx *ctx;
> >         int err;
> >
> > @@ -899,6 +900,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> >                 return;
> >
> >         ctx = xsk->ctx;
> > +       umem = ctx->umem;
> >         if (ctx->prog_fd != -1) {
> >                 xsk_delete_bpf_maps(xsk);
> >                 close(ctx->prog_fd);
> > @@ -918,11 +920,11 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> >
> >         xsk_put_ctx(ctx);
> >
> > -       ctx->umem->refcount--;
> > +       umem->refcount--;
>
> if you moved ctx->umem->refcount--; to before xdk_put_ctx(ctx), would
> that also work?

Yes, it would for that statement, but I still need the umem pointer
for the statement below. And this statement of potentially closing the
fd needs to be after xsk_put_ctx(). So we might as well keep
ujmem->refcount-- where it is, if that is ok with you?

> >         /* Do not close an fd that also has an associated umem connected
> >          * to it.
> >          */
> > -       if (xsk->fd != ctx->umem->fd)
> > +       if (xsk->fd != umem->fd)
> >                 close(xsk->fd);
> >         free(xsk);
> >  }
> > --
> > 2.7.4
> >

Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete

[Andrii Nakryiko]

On Wed, Nov 4, 2020 at 12:27 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> On Tue, Nov 3, 2020 at 8:05 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Nov 3, 2020 at 1:42 AM Magnus Karlsson
> > <magnus.karlsson@gmail.com> wrote:
> > >
> > > From: Magnus Karlsson <magnus.karlsson@intel.com>
> > >
> > > Fix a possible use after free in xsk_socket__delete that will happen
> > > if xsk_put_ctx() frees the ctx. To fix, save the umem reference taken
> > > from the context and just use that instead.
> > >
> > > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices")
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > > ---
> > >  tools/lib/bpf/xsk.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > index 504b7a8..9bc537d 100644
> > > --- a/tools/lib/bpf/xsk.c
> > > +++ b/tools/lib/bpf/xsk.c
> > > @@ -892,6 +892,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> > >  {
> > >         size_t desc_sz = sizeof(struct xdp_desc);
> > >         struct xdp_mmap_offsets off;
> > > +       struct xsk_umem *umem;
> > >         struct xsk_ctx *ctx;
> > >         int err;
> > >
> > > @@ -899,6 +900,7 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> > >                 return;
> > >
> > >         ctx = xsk->ctx;
> > > +       umem = ctx->umem;
> > >         if (ctx->prog_fd != -1) {
> > >                 xsk_delete_bpf_maps(xsk);
> > >                 close(ctx->prog_fd);
> > > @@ -918,11 +920,11 @@ void xsk_socket__delete(struct xsk_socket *xsk)
> > >
> > >         xsk_put_ctx(ctx);
> > >
> > > -       ctx->umem->refcount--;
> > > +       umem->refcount--;
> >
> > if you moved ctx->umem->refcount--; to before xdk_put_ctx(ctx), would
> > that also work?
>
> Yes, it would for that statement, but I still need the umem pointer
> for the statement below. And this statement of potentially closing the
> fd needs to be after xsk_put_ctx(). So we might as well keep
> ujmem->refcount-- where it is, if that is ok with you?

Ah, missed the umem->fd below. Then it makes sense, thanks.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
> > >         /* Do not close an fd that also has an associated umem connected
> > >          * to it.
> > >          */
> > > -       if (xsk->fd != ctx->umem->fd)
> > > +       if (xsk->fd != umem->fd)
> > >                 close(xsk->fd);
> > >         free(xsk);
> > >  }
> > > --
> > > 2.7.4
> > >