| Message ID | 1604396490-12129-3-git-send-email-magnus.karlsson@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | libbpf: fix two bugs in xsk_socket__delete | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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 >
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 > >
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 > > >
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); }