netdev - Re: [Patch bpf] bpf: fix a potential use-after-free in bpf_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 3 Jun 2024 18:15:55 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jiri Olsa <olsajiri@...il.com>, Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
 Cong Wang <cong.wang@...edance.com>,
 syzbot+1989ee16d94720836244@...kaller.appspotmail.com,
 Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [Patch bpf] bpf: fix a potential use-after-free in
 bpf_link_free()

On 6/2/24 9:32 PM, Jiri Olsa wrote:
> On Sun, Jun 02, 2024 at 11:27:03AM -0700, Cong Wang wrote:
>> From: Cong Wang <cong.wang@...edance.com>
>>
>> After commit 1a80dbcb2dba, bpf_link can be freed by
>> link->ops->dealloc_deferred, but the code still tests and uses
>> link->ops->dealloc afterward, which leads to a use-after-free as
>> reported by syzbot. Actually, one of them should be sufficient, so
>> just call one of them instead of both. Also add a WARN_ON() in case
>> of any problematic implementation.
>>
>> Reported-by: syzbot+1989ee16d94720836244@...kaller.appspotmail.com
>> Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
>> Cc: Andrii Nakryiko <andrii@...nel.org>
>> Signed-off-by: Cong Wang <cong.wang@...edance.com>
>> ---
>>   kernel/bpf/syscall.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 2222c3ff88e7..d8f244069495 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -2998,6 +2998,7 @@ static int bpf_obj_get(const union bpf_attr *attr)
>>   void bpf_link_init(struct bpf_link *link, enum bpf_link_type type,
>>   		   const struct bpf_link_ops *ops, struct bpf_prog *prog)
>>   {
>> +	WARN_ON(ops->dealloc && ops->dealloc_deferred);
>>   	atomic64_set(&link->refcnt, 1);
>>   	link->type = type;
>>   	link->id = 0;
>> @@ -3074,8 +3075,7 @@ static void bpf_link_free(struct bpf_link *link)
>>   			call_rcu_tasks_trace(&link->rcu, bpf_link_defer_dealloc_mult_rcu_gp);
>>   		else
>>   			call_rcu(&link->rcu, bpf_link_defer_dealloc_rcu_gp);
>> -	}
>> -	if (link->ops->dealloc)
>> +	} else if (link->ops->dealloc)
>>   		link->ops->dealloc(link);
> 
> nice catch

+1, thanks Cong !

> Acked-by: Jiri Olsa <jolsa@...nel.org>

I think it would also be slightly nicer to just fetch the ops once, which
wouldn't have caused the issue if it was done back then in the first place.
Do you mind if I squash this in and then apply it to bpf tree? Looks as
follows :

 From 220fb40d88c7b5d1f3234a5f1b807f6067450832 Mon Sep 17 00:00:00 2001
Message-Id: <220fb40d88c7b5d1f3234a5f1b807f6067450832.1717431085.git.daniel@...earbox.net>
From: Cong Wang <cong.wang@...edance.com>
Date: Sun, 2 Jun 2024 11:27:03 -0700
Subject: [PATCH bpf] bpf: Fix a potential use-after-free in bpf_link_free()

After commit 1a80dbcb2dba, bpf_link can be freed by
link->ops->dealloc_deferred, but the code still tests and uses
link->ops->dealloc afterward, which leads to a use-after-free as
reported by syzbot. Actually, one of them should be sufficient, so
just call one of them instead of both. Also add a WARN_ON() in case
of any problematic implementation.

Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
Reported-by: syzbot+1989ee16d94720836244@...kaller.appspotmail.com
Signed-off-by: Cong Wang <cong.wang@...edance.com>
Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
Acked-by: Jiri Olsa <jolsa@...nel.org>
Link: https://lore.kernel.org/bpf/20240602182703.207276-1-xiyou.wangcong@gmail.com
---
  kernel/bpf/syscall.c | 11 ++++++-----
  1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5070fa20d05c..869265852d51 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2998,6 +2998,7 @@ static int bpf_obj_get(const union bpf_attr *attr)
  void bpf_link_init(struct bpf_link *link, enum bpf_link_type type,
  		   const struct bpf_link_ops *ops, struct bpf_prog *prog)
  {
+	WARN_ON(ops->dealloc && ops->dealloc_deferred);
  	atomic64_set(&link->refcnt, 1);
  	link->type = type;
  	link->id = 0;
@@ -3056,16 +3057,17 @@ static void bpf_link_defer_dealloc_mult_rcu_gp(struct rcu_head *rcu)
  /* bpf_link_free is guaranteed to be called from process context */
  static void bpf_link_free(struct bpf_link *link)
  {
+	const struct bpf_link_ops *ops = link->ops;
  	bool sleepable = false;

  	bpf_link_free_id(link->id);
  	if (link->prog) {
  		sleepable = link->prog->sleepable;
  		/* detach BPF program, clean up used resources */
-		link->ops->release(link);
+		ops->release(link);
  		bpf_prog_put(link->prog);
  	}
-	if (link->ops->dealloc_deferred) {
+	if (ops->dealloc_deferred) {
  		/* schedule BPF link deallocation; if underlying BPF program
  		 * is sleepable, we need to first wait for RCU tasks trace
  		 * sync, then go through "classic" RCU grace period
@@ -3074,9 +3076,8 @@ static void bpf_link_free(struct bpf_link *link)
  			call_rcu_tasks_trace(&link->rcu, bpf_link_defer_dealloc_mult_rcu_gp);
  		else
  			call_rcu(&link->rcu, bpf_link_defer_dealloc_rcu_gp);
-	}
-	if (link->ops->dealloc)
-		link->ops->dealloc(link);
+	} else if (ops->dealloc)
+		ops->dealloc(link);
  }

  static void bpf_link_put_deferred(struct work_struct *work)
-- 
2.21.0