[PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Waiman Long]

Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
writeback has finished") delayed call to blkcg_destroy_blkgs() to
cgwb_release_workfn(). However, it is done after a css_put() of blkcg
which may be the final put that causes the blkcg to be freed as RCU
read lock isn't held.

By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
failure, the following stack trace was produced in a test system on
bootup.

[   34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
      :
[   34.339943] Call Trace:
[   34.342395]  <TASK>
[   34.344510]  blkcg_unpin_online+0x38/0x60
[   34.348523]  cgwb_release_workfn+0x6a/0x200
[   34.352708]  process_one_work+0x1e5/0x3b0
[   34.356742]  ? rescuer_thread+0x390/0x390
[   34.360758]  worker_thread+0x50/0x3a0
[   34.364425]  ? rescuer_thread+0x390/0x390
[   34.368447]  kthread+0xd9/0x100
[   34.371592]  ? kthread_complete_and_exit+0x20/0x20
[   34.376386]  ret_from_fork+0x22/0x30
[   34.379982]  </TASK>

This confirms that a potential UAF situation can happen.

Fix that by delaying the css_put() until after the blkcg_unpin_online()
call. Also use css_tryget() in blkcg_destroy_blkgs() and issue a warning
if css_tryget() fails with no RCU read lock held.

The reproducing system can no longer produce a warning with this patch.
All the runnable block/0* tests including block/027 were run successfully
without failure.

Fixes: 59b57717fff8 ("blkcg: delay blkg destruction until after writeback has finished")
Suggested-by: Michal Koutný <mkoutny@suse.com>
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 block/blk-cgroup.c | 10 +++++++++-
 mm/backing-dev.c   |  8 ++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 57941d2a8ba3..904372bb96f1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
 
 	might_sleep();
 
-	css_get(&blkcg->css);
+	/*
+	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
+	 * references gone and rcu_read_lock not held.
+	 */
+	if (!css_tryget(&blkcg->css)) {
+		WARN_ON_ONCE(!rcu_read_lock_held());
+		return;
+	}
+
 	spin_lock_irq(&blkcg->lock);
 	while (!hlist_empty(&blkcg->blkg_list)) {
 		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c30419a5e119..36f75b072325 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -390,11 +390,15 @@ static void cgwb_release_workfn(struct work_struct *work)
 	wb_shutdown(wb);
 
 	css_put(wb->memcg_css);
-	css_put(wb->blkcg_css);
 	mutex_unlock(&wb->bdi->cgwb_release_mutex);
 
-	/* triggers blkg destruction if no online users left */
+	/*
+	 * Triggers blkg destruction if no online users left
+	 * The final blkcg css_put() has to be done after blkcg_unpin_online()
+	 * to avoid use-after-free.
+	 */
 	blkcg_unpin_online(wb->blkcg_css);
+	css_put(wb->blkcg_css);
 
 	fprop_local_destroy_percpu(&wb->memcg_completions);
 
-- 
2.31.1

[PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Waiman Long]

Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
writeback has finished") delayed call to blkcg_destroy_blkgs() to
cgwb_release_workfn(). However, it is done after a css_put() of blkcg
which may be the final put that causes the blkcg to be freed as RCU
read lock isn't held.

By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
failure, the following stack trace was produced in a test system on
bootup.

[   34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
      :
[   34.339943] Call Trace:
[   34.342395]  <TASK>
[   34.344510]  blkcg_unpin_online+0x38/0x60
[   34.348523]  cgwb_release_workfn+0x6a/0x200
[   34.352708]  process_one_work+0x1e5/0x3b0
[   34.356742]  ? rescuer_thread+0x390/0x390
[   34.360758]  worker_thread+0x50/0x3a0
[   34.364425]  ? rescuer_thread+0x390/0x390
[   34.368447]  kthread+0xd9/0x100
[   34.371592]  ? kthread_complete_and_exit+0x20/0x20
[   34.376386]  ret_from_fork+0x22/0x30
[   34.379982]  </TASK>

This confirms that a potential UAF situation can happen.

Fix that by delaying the css_put() until after the blkcg_unpin_online()
call. Also use css_tryget() in blkcg_destroy_blkgs() and issue a warning
if css_tryget() fails with no RCU read lock held.

The reproducing system can no longer produce a warning with this patch.
All the runnable block/0* tests including block/027 were run successfully
without failure.

Fixes: 59b57717fff8 ("blkcg: delay blkg destruction until after writeback has finished")
Suggested-by: Michal Koutn√Ω <mkoutny@suse.com>
Reported-by: Yi Zhang <yi.zhang@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 block/blk-cgroup.c | 10 +++++++++-
 mm/backing-dev.c   |  8 ++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 57941d2a8ba3..904372bb96f1 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
 
 	might_sleep();
 
-	css_get(&blkcg->css);
+	/*
+	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
+	 * references gone and rcu_read_lock not held.
+	 */
+	if (!css_tryget(&blkcg->css)) {
+		WARN_ON_ONCE(!rcu_read_lock_held());
+		return;
+	}
+
 	spin_lock_irq(&blkcg->lock);
 	while (!hlist_empty(&blkcg->blkg_list)) {
 		struct blkcg_gq *blkg = hlist_entry(blkcg->blkg_list.first,
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c30419a5e119..36f75b072325 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -390,11 +390,15 @@ static void cgwb_release_workfn(struct work_struct *work)
 	wb_shutdown(wb);
 
 	css_put(wb->memcg_css);
-	css_put(wb->blkcg_css);
 	mutex_unlock(&wb->bdi->cgwb_release_mutex);
 
-	/* triggers blkg destruction if no online users left */
+	/*
+	 * Triggers blkg destruction if no online users left
+	 * The final blkcg css_put() has to be done after blkcg_unpin_online()
+	 * to avoid use-after-free.
+	 */
 	blkcg_unpin_online(wb->blkcg_css);
+	css_put(wb->blkcg_css);
 
 	fprop_local_destroy_percpu(&wb->memcg_completions);
 
-- 
2.31.1

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long wrote:
> Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
> writeback has finished") delayed call to blkcg_destroy_blkgs() to
> cgwb_release_workfn(). However, it is done after a css_put() of blkcg
> which may be the final put that causes the blkcg to be freed as RCU
> read lock isn't held.
> 
> By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
> failure, the following stack trace was produced in a test system on
> bootup.
> 
> [   34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
>       :
> [   34.339943] Call Trace:
> [   34.342395]  <TASK>
> [   34.344510]  blkcg_unpin_online+0x38/0x60
> [   34.348523]  cgwb_release_workfn+0x6a/0x200
> [   34.352708]  process_one_work+0x1e5/0x3b0
> [   34.356742]  ? rescuer_thread+0x390/0x390
> [   34.360758]  worker_thread+0x50/0x3a0
> [   34.364425]  ? rescuer_thread+0x390/0x390
> [   34.368447]  kthread+0xd9/0x100
> [   34.371592]  ? kthread_complete_and_exit+0x20/0x20
> [   34.376386]  ret_from_fork+0x22/0x30
> [   34.379982]  </TASK>

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages


> This confirms that a potential UAF situation can happen.
> 
> Fix that by delaying the css_put() until after the blkcg_unpin_online()
> call. Also use css_tryget() in blkcg_destroy_blkgs() and issue a warning
> if css_tryget() fails with no RCU read lock held.
> 
> The reproducing system can no longer produce a warning with this patch.
> All the runnable block/0* tests including block/027 were run successfully
> without failure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long wrote:
> Commit 59b57717fff8 ("blkcg: delay blkg destruction until after
> writeback has finished") delayed call to blkcg_destroy_blkgs() to
> cgwb_release_workfn(). However, it is done after a css_put() of blkcg
> which may be the final put that causes the blkcg to be freed as RCU
> read lock isn't held.
> 
> By adding a css_tryget() into blkcg_destroy_blkgs() and warning its
> failure, the following stack trace was produced in a test system on
> bootup.
> 
> [   34.254240] RIP: 0010:blkcg_destroy_blkgs+0x16a/0x1a0
>       :
> [   34.339943] Call Trace:
> [   34.342395]  <TASK>
> [   34.344510]  blkcg_unpin_online+0x38/0x60
> [   34.348523]  cgwb_release_workfn+0x6a/0x200
> [   34.352708]  process_one_work+0x1e5/0x3b0
> [   34.356742]  ? rescuer_thread+0x390/0x390
> [   34.360758]  worker_thread+0x50/0x3a0
> [   34.364425]  ? rescuer_thread+0x390/0x390
> [   34.368447]  kthread+0xd9/0x100
> [   34.371592]  ? kthread_complete_and_exit+0x20/0x20
> [   34.376386]  ret_from_fork+0x22/0x30
> [   34.379982]  </TASK>

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages


> This confirms that a potential UAF situation can happen.
> 
> Fix that by delaying the css_put() until after the blkcg_unpin_online()
> call. Also use css_tryget() in blkcg_destroy_blkgs() and issue a warning
> if css_tryget() fails with no RCU read lock held.
> 
> The reproducing system can no longer produce a warning with this patch.
> All the runnable block/0* tests including block/027 were run successfully
> without failure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman@redhat.com> wrote:
> The reproducing system can no longer produce a warning with this patch.
> All the runnable block/0* tests including block/027 were run successfully
> without failure.

Thanks for the test!

> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>  
>  	might_sleep();
>  
> -	css_get(&blkcg->css);
> +	/*
> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
> +	 * references gone and rcu_read_lock not held.
> +	 */
> +	if (!css_tryget(&blkcg->css)) {
> +		WARN_ON_ONCE(!rcu_read_lock_held());
> +		return;
> +	}

As I followed the previous discussion, the principle is that obtaining a
reference or being inside an RCU read section is sufficient.

Consequently, I'd expect the two situations handled equally but here the
no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)

However, the might_sleep() in (non-sleepable) RCU reader section combo
makes me wary anyway (not with the early return but tools would likely
complain).

All in all, can't the contract of blkcg_destroy_blkgs() declare that
a caller must pass blkcg with a valid reference? (The body of
blkcg_destroy_blkgs then wouldn't need to get neither put the inner
reference).

HTH,
Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The reproducing system can no longer produce a warning with this patch.
> All the runnable block/0* tests including block/027 were run successfully
> without failure.

Thanks for the test!

> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>  
>  	might_sleep();
>  
> -	css_get(&blkcg->css);
> +	/*
> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
> +	 * references gone and rcu_read_lock not held.
> +	 */
> +	if (!css_tryget(&blkcg->css)) {
> +		WARN_ON_ONCE(!rcu_read_lock_held());
> +		return;
> +	}

As I followed the previous discussion, the principle is that obtaining a
reference or being inside an RCU read section is sufficient.

Consequently, I'd expect the two situations handled equally but here the
no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)

However, the might_sleep() in (non-sleepable) RCU reader section combo
makes me wary anyway (not with the early return but tools would likely
complain).

All in all, can't the contract of blkcg_destroy_blkgs() declare that
a caller must pass blkcg with a valid reference? (The body of
blkcg_destroy_blkgs then wouldn't need to get neither put the inner
reference).

HTH,
Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Jens Axboe]

On 11/30/22 8:16?AM, Michal Koutn? wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman@redhat.com> wrote:
>> The reproducing system can no longer produce a warning with this patch.
>> All the runnable block/0* tests including block/027 were run successfully
>> without failure.
> 
> Thanks for the test!
> 
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>  
>>  	might_sleep();
>>  
>> -	css_get(&blkcg->css);
>> +	/*
>> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> +	 * references gone and rcu_read_lock not held.
>> +	 */
>> +	if (!css_tryget(&blkcg->css)) {
>> +		WARN_ON_ONCE(!rcu_read_lock_held());
>> +		return;
>> +	}
> 
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
> 
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
> 
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
> 
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).

Totally agree, the proposed patch feels more like a hacky workaround
rather than a true solution. Either the contract should be that it's
ALWAYS entered with RCU lock held and hence the tryget is fine, OR that
a reference always is held when entered.

I'm going to revert the offending patch for now, and then we can queue
up a proper patch when that exists.

-- 
Jens Axboe

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Jens Axboe]

On 11/30/22 8:16?AM, Michal Koutn? wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> The reproducing system can no longer produce a warning with this patch.
>> All the runnable block/0* tests including block/027 were run successfully
>> without failure.
> 
> Thanks for the test!
> 
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>  
>>  	might_sleep();
>>  
>> -	css_get(&blkcg->css);
>> +	/*
>> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> +	 * references gone and rcu_read_lock not held.
>> +	 */
>> +	if (!css_tryget(&blkcg->css)) {
>> +		WARN_ON_ONCE(!rcu_read_lock_held());
>> +		return;
>> +	}
> 
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
> 
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
> 
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
> 
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).

Totally agree, the proposed patch feels more like a hacky workaround
rather than a true solution. Either the contract should be that it's
ALWAYS entered with RCU lock held and hence the tryget is fine, OR that
a reference always is held when entered.

I'm going to revert the offending patch for now, and then we can queue
up a proper patch when that exists.

-- 
Jens Axboe

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Waiman Long]

On 11/30/22 10:16, Michal Koutný wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman@redhat.com> wrote:
>> The reproducing system can no longer produce a warning with this patch.
>> All the runnable block/0* tests including block/027 were run successfully
>> without failure.
> Thanks for the test!
>
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>   
>>   	might_sleep();
>>   
>> -	css_get(&blkcg->css);
>> +	/*
>> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> +	 * references gone and rcu_read_lock not held.
>> +	 */
>> +	if (!css_tryget(&blkcg->css)) {
>> +		WARN_ON_ONCE(!rcu_read_lock_held());
>> +		return;
>> +	}
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
>
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
>
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
>
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).

You are right. I should have pushed the might_sleep down(). Will post a 
new version to fix that.

Thanks,
Longman

Re: [PATCH-block v2] bdi, blk-cgroup: Fix potential UAF of blkcg

[Waiman Long]

On 11/30/22 10:16, Michal Koutný wrote:
> On Tue, Nov 29, 2022 at 03:34:00PM -0500, Waiman Long <longman@redhat.com> wrote:
>> The reproducing system can no longer produce a warning with this patch.
>> All the runnable block/0* tests including block/027 were run successfully
>> without failure.
> Thanks for the test!
>
>> @@ -1088,7 +1088,15 @@ static void blkcg_destroy_blkgs(struct blkcg *blkcg)
>>   
>>   	might_sleep();
>>   
>> -	css_get(&blkcg->css);
>> +	/*
>> +	 * blkcg_destroy_blkgs() shouldn't be called with all the blkcg
>> +	 * references gone and rcu_read_lock not held.
>> +	 */
>> +	if (!css_tryget(&blkcg->css)) {
>> +		WARN_ON_ONCE(!rcu_read_lock_held());
>> +		return;
>> +	}
> As I followed the previous discussion, the principle is that obtaining a
> reference or being inside an RCU read section is sufficient.
>
> Consequently, I'd expect the two situations handled equally but here the
> no-ref but RCU bails out. (Which is OK because blkg_list must be empty?)
>
> However, the might_sleep() in (non-sleepable) RCU reader section combo
> makes me wary anyway (not with the early return but tools would likely
> complain).
>
> All in all, can't the contract of blkcg_destroy_blkgs() declare that
> a caller must pass blkcg with a valid reference? (The body of
> blkcg_destroy_blkgs then wouldn't need to get neither put the inner
> reference).

You are right. I should have pushed the might_sleep down(). Will post a 
new version to fix that.

Thanks,
Longman