| Message ID | 20200501185622.3088964-1-andriin@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [v2,bpf-next] bpf: fix use-after-free of bpf_link when priming half-fails | expand |
On Fri, May 01, 2020 at 11:56:22AM -0700, Andrii Nakryiko wrote: > If bpf_link_prime() succeeds to allocate new anon file, but then fails to > allocate ID for it, link priming is considered to be failed and user is > supposed ot be able to directly kfree() bpf_link, because it was never exposed > to user-space. > > But at that point file already keeps a pointer to bpf_link and will eventually > call bpf_link_release(), so if bpf_link was kfree()'d by caller, that would > lead to use-after-free. > > Fix this by first allocating ID and only then allocating file. Adding ID to > link_idr is ok, because link at that point still doesn't have its ID set, so > no user-space process can create a new FD for it. Acked-by: Martin KaFai Lau <kafai@fb.com>
On Fri, May 1, 2020 at 1:00 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Fri, May 01, 2020 at 11:56:22AM -0700, Andrii Nakryiko wrote: > > If bpf_link_prime() succeeds to allocate new anon file, but then fails to > > allocate ID for it, link priming is considered to be failed and user is > > supposed ot be able to directly kfree() bpf_link, because it was never exposed > > to user-space. > > > > But at that point file already keeps a pointer to bpf_link and will eventually > > call bpf_link_release(), so if bpf_link was kfree()'d by caller, that would > > lead to use-after-free. > > > > Fix this by first allocating ID and only then allocating file. Adding ID to > > link_idr is ok, because link at that point still doesn't have its ID set, so > > no user-space process can create a new FD for it. > Acked-by: Martin KaFai Lau <kafai@fb.com> Applied. Thanks
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index c75b2dd2459c..108c8051dff2 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2348,19 +2348,20 @@ int bpf_link_prime(struct bpf_link *link, struct bpf_link_primer *primer) if (fd < 0) return fd; - file = anon_inode_getfile("bpf_link", &bpf_link_fops, link, O_CLOEXEC); - if (IS_ERR(file)) { - put_unused_fd(fd); - return PTR_ERR(file); - } id = bpf_link_alloc_id(link); if (id < 0) { put_unused_fd(fd); - fput(file); return id; } + file = anon_inode_getfile("bpf_link", &bpf_link_fops, link, O_CLOEXEC); + if (IS_ERR(file)) { + bpf_link_free_id(id); + put_unused_fd(fd); + return PTR_ERR(file); + } + primer->link = link; primer->file = file; primer->fd = fd;
If bpf_link_prime() succeeds to allocate new anon file, but then fails to allocate ID for it, link priming is considered to be failed and user is supposed ot be able to directly kfree() bpf_link, because it was never exposed to user-space. But at that point file already keeps a pointer to bpf_link and will eventually call bpf_link_release(), so if bpf_link was kfree()'d by caller, that would lead to use-after-free. Fix this by first allocating ID and only then allocating file. Adding ID to link_idr is ok, because link at that point still doesn't have its ID set, so no user-space process can create a new FD for it. Suggested-by: Martin KaFai Lau <kafai@fb.com> Fixes: a3b80e107894 ("bpf: Allocate ID for bpf_link") Reported-by: syzbot+39b64425f91b5aab714d@syzkaller.appspotmail.com Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- kernel/bpf/syscall.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)