| Message ID | 20221129203400.1456100-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-block,v2] bdi, blk-cgroup: Fix potential UAF of blkcg | expand |
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.
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
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.
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
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);
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(-)