[bpf] bpf: fix a potential use-after-free in bpf_link_free()

Commit Message

Cong Wang June 2, 2024, 6:27 p.m. UTC

From: Cong Wang <[e-mail]>

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: [e-mail]
Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
Cc: Andrii Nakryiko <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 kernel/bpf/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jiri Olsa June 2, 2024, 7:32 p.m. UTC | #1

On Sun, Jun 02, 2024 at 11:27:03AM -0700, Cong Wang wrote:
> From: Cong Wang <[e-mail]>
> 
> 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: [e-mail]
> Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
> Cc: Andrii Nakryiko <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---
>  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

Acked-by: Jiri Olsa <[e-mail]>

jirka

Daniel Borkmann June 3, 2024, 4:15 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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: [e-mail]
>> Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
>> Cc: Andrii Nakryiko <[e-mail]>
>> Signed-off-by: Cong Wang <[e-mail]>
>> ---
>>   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 <[e-mail]>

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: <[e-mail]>
From: Cong Wang <[e-mail]>
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: [e-mail]
Signed-off-by: Cong Wang <[e-mail]>
Signed-off-by: Daniel Borkmann <[e-mail]>
Acked-by: Jiri Olsa <[e-mail]>
Link: https://lore.kernel.org/bpf/[e-mail]
---
  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)

Cong Wang June 4, 2024, 3:22 p.m. UTC | #3

On Mon, Jun 03, 2024 at 06:15:55PM +0200, Daniel Borkmann wrote:
> 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 <[e-mail]>
> > > 
> > > 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: [e-mail]
> > > Fixes: 1a80dbcb2dba ("bpf: support deferring bpf_link dealloc to after RCU grace period")
> > > Cc: Andrii Nakryiko <[e-mail]>
> > > Signed-off-by: Cong Wang <[e-mail]>
> > > ---
> > >   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 <[e-mail]>
> 
> 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 :
> 

Sounds good to me.

Thanks.

[e-mail] June 4, 2024, 3:50 p.m. UTC | #4

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <[e-mail]>:

On Sun,  2 Jun 2024 11:27:03 -0700 you wrote:
> From: Cong Wang <[e-mail]>
> 
> 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.
> 
> [...]

Here is the summary with links:
  - [bpf] bpf: fix a potential use-after-free in bpf_link_free()
    https://git.kernel.org/bpf/bpf/c/2884dc7d08d9

You are awesome, thank you!

Patch

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);
 }