* [PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete
@ 2020-11-03 9:41 Magnus Karlsson
2020-11-03 9:41 ` [PATCH bpf 1/2] libbpf: fix null dereference " Magnus Karlsson
2020-11-03 9:41 ` [PATCH bpf 2/2] libbpf: fix possible use after free " Magnus Karlsson
0 siblings, 2 replies; 7+ messages in thread
From: Magnus Karlsson @ 2020-11-03 9:41 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev,
jonathan.lemon, andrii.nakryiko
Cc: Magnus Karlsson, bpf
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH bpf 1/2] libbpf: fix null dereference in xsk_socket__delete 2020-11-03 9:41 [PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete Magnus Karlsson @ 2020-11-03 9:41 ` Magnus Karlsson 2020-11-03 19:02 ` Andrii Nakryiko 2020-11-03 9:41 ` [PATCH bpf 2/2] libbpf: fix possible use after free " Magnus Karlsson 1 sibling, 1 reply; 7+ messages in thread From: Magnus Karlsson @ 2020-11-03 9:41 UTC (permalink / raw) To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jonathan.lemon, andrii.nakryiko Cc: bpf 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 1/2] libbpf: fix null dereference in xsk_socket__delete 2020-11-03 9:41 ` [PATCH bpf 1/2] libbpf: fix null dereference " Magnus Karlsson @ 2020-11-03 19:02 ` Andrii Nakryiko 0 siblings, 0 replies; 7+ messages in thread From: Andrii Nakryiko @ 2020-11-03 19:02 UTC (permalink / raw) To: Magnus Karlsson Cc: Magnus Karlsson, Björn Töpel, Alexei Starovoitov, Daniel Borkmann, Networking, Jonathan Lemon, bpf 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete 2020-11-03 9:41 [PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete Magnus Karlsson 2020-11-03 9:41 ` [PATCH bpf 1/2] libbpf: fix null dereference " Magnus Karlsson @ 2020-11-03 9:41 ` Magnus Karlsson 2020-11-03 19:05 ` Andrii Nakryiko 1 sibling, 1 reply; 7+ messages in thread From: Magnus Karlsson @ 2020-11-03 9:41 UTC (permalink / raw) To: magnus.karlsson, bjorn.topel, ast, daniel, netdev, jonathan.lemon, andrii.nakryiko Cc: bpf 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete 2020-11-03 9:41 ` [PATCH bpf 2/2] libbpf: fix possible use after free " Magnus Karlsson @ 2020-11-03 19:05 ` Andrii Nakryiko 2020-11-04 8:26 ` Magnus Karlsson 0 siblings, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2020-11-03 19:05 UTC (permalink / raw) To: Magnus Karlsson Cc: Magnus Karlsson, Björn Töpel, Alexei Starovoitov, Daniel Borkmann, Networking, Jonathan Lemon, bpf 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete 2020-11-03 19:05 ` Andrii Nakryiko @ 2020-11-04 8:26 ` Magnus Karlsson 2020-11-04 19:42 ` Andrii Nakryiko 0 siblings, 1 reply; 7+ messages in thread From: Magnus Karlsson @ 2020-11-04 8:26 UTC (permalink / raw) To: Andrii Nakryiko Cc: Magnus Karlsson, Björn Töpel, Alexei Starovoitov, Daniel Borkmann, Networking, Jonathan Lemon, bpf 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 2/2] libbpf: fix possible use after free in xsk_socket__delete 2020-11-04 8:26 ` Magnus Karlsson @ 2020-11-04 19:42 ` Andrii Nakryiko 0 siblings, 0 replies; 7+ messages in thread From: Andrii Nakryiko @ 2020-11-04 19:42 UTC (permalink / raw) To: Magnus Karlsson Cc: Magnus Karlsson, Björn Töpel, Alexei Starovoitov, Daniel Borkmann, Networking, Jonathan Lemon, bpf 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-04 19:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-03 9:41 [PATCH bpf 0/2] libbpf: fix two bugs in xsk_socket__delete Magnus Karlsson 2020-11-03 9:41 ` [PATCH bpf 1/2] libbpf: fix null dereference " Magnus Karlsson 2020-11-03 19:02 ` Andrii Nakryiko 2020-11-03 9:41 ` [PATCH bpf 2/2] libbpf: fix possible use after free " Magnus Karlsson 2020-11-03 19:05 ` Andrii Nakryiko 2020-11-04 8:26 ` Magnus Karlsson 2020-11-04 19:42 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).