| Message ID | 20230308023208.379465-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, bfq: fix uaf for 'stable_merge_bfqq' | expand |
On Mar 08, 2023 / 10:32, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array > in bfq_io_cq"), process reference is read before bfq_put_stable_ref(), > and it's safe if bfq_put_stable_ref() put the last reference, because > process reference will be 0 and 'stable_merge_bfqq' won't be accessed > in this case. However, the commit changed the order and will cause > uaf for 'stable_merge_bfqq'. > > In order to emphasize that bfq_put_stable_ref() can drop the last > reference, fix the problem by moving bfq_put_stable_ref() to the end of > bfq_setup_stable_merge(). > > Fixes: fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Link: https://lore.kernel.org/linux-block/20230307071448.rzihxbm4jhbf5krj@shindev/ > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-iosched.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 8a8d4441519c..d9ed3108c17a 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, > { > int proc_ref = min(bfqq_process_refs(bfqq), > bfqq_process_refs(stable_merge_bfqq)); > - struct bfq_queue *new_bfqq; > + struct bfq_queue *new_bfqq = NULL; > > - if (idling_boosts_thr_without_issues(bfqd, bfqq) || > - proc_ref == 0) > - return NULL; > + bfqq_data->stable_merge_bfqq = NULL; > + if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0) > + goto out; > > /* next function will take at least one ref */ > new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq); > @@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, > new_bfqq_data->stably_merged = true; > } > } > + > +out: > + /* deschedule stable merge, because done or aborted here */ > + bfq_put_stable_ref(stable_merge_bfqq); > + > return new_bfqq; > } > > @@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, > struct bfq_queue *stable_merge_bfqq = > bfqq_data->stable_merge_bfqq; Nit: I suggest to remove the local variable above. The two references to it below are removed, and the argument of bfq_setup_stable_merge() can be replaced with bfqq_data->stable_merge_bfqq. > > - /* deschedule stable merge, because done or aborted here */ > - bfq_put_stable_ref(stable_merge_bfqq); > - > - bfqq_data->stable_merge_bfqq = NULL; > - > return bfq_setup_stable_merge(bfqd, bfqq, > stable_merge_bfqq, > bfqq_data); > -- > 2.31.1 >
On Wed 08-03-23 10:32:08, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array > in bfq_io_cq"), process reference is read before bfq_put_stable_ref(), > and it's safe if bfq_put_stable_ref() put the last reference, because > process reference will be 0 and 'stable_merge_bfqq' won't be accessed > in this case. However, the commit changed the order and will cause > uaf for 'stable_merge_bfqq'. > > In order to emphasize that bfq_put_stable_ref() can drop the last > reference, fix the problem by moving bfq_put_stable_ref() to the end of > bfq_setup_stable_merge(). > > Fixes: fd571df0ac5b ("block, bfq: turn bfqq_data into an array in bfq_io_cq") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Link: https://lore.kernel.org/linux-block/20230307071448.rzihxbm4jhbf5krj@shindev/ > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Looks good to me (with or without getting rid of the stable_merge_bfqq) variable. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/bfq-iosched.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 8a8d4441519c..d9ed3108c17a 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, > { > int proc_ref = min(bfqq_process_refs(bfqq), > bfqq_process_refs(stable_merge_bfqq)); > - struct bfq_queue *new_bfqq; > + struct bfq_queue *new_bfqq = NULL; > > - if (idling_boosts_thr_without_issues(bfqd, bfqq) || > - proc_ref == 0) > - return NULL; > + bfqq_data->stable_merge_bfqq = NULL; > + if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0) > + goto out; > > /* next function will take at least one ref */ > new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq); > @@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, > new_bfqq_data->stably_merged = true; > } > } > + > +out: > + /* deschedule stable merge, because done or aborted here */ > + bfq_put_stable_ref(stable_merge_bfqq); > + > return new_bfqq; > } > > @@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, > struct bfq_queue *stable_merge_bfqq = > bfqq_data->stable_merge_bfqq; > > - /* deschedule stable merge, because done or aborted here */ > - bfq_put_stable_ref(stable_merge_bfqq); > - > - bfqq_data->stable_merge_bfqq = NULL; > - > return bfq_setup_stable_merge(bfqd, bfqq, > stable_merge_bfqq, > bfqq_data); > -- > 2.31.1 >
On Wed, 08 Mar 2023 10:32:08 +0800, Yu Kuai wrote: > Before commit fd571df0ac5b ("block, bfq: turn bfqq_data into an array > in bfq_io_cq"), process reference is read before bfq_put_stable_ref(), > and it's safe if bfq_put_stable_ref() put the last reference, because > process reference will be 0 and 'stable_merge_bfqq' won't be accessed > in this case. However, the commit changed the order and will cause > uaf for 'stable_merge_bfqq'. > > [...] Applied, thanks! [1/1] block, bfq: fix uaf for 'stable_merge_bfqq' commit: e2f2a39452c43b64ea3191642a2661cb8d03827a Best regards,
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 8a8d4441519c..d9ed3108c17a 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2854,11 +2854,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, { int proc_ref = min(bfqq_process_refs(bfqq), bfqq_process_refs(stable_merge_bfqq)); - struct bfq_queue *new_bfqq; + struct bfq_queue *new_bfqq = NULL; - if (idling_boosts_thr_without_issues(bfqd, bfqq) || - proc_ref == 0) - return NULL; + bfqq_data->stable_merge_bfqq = NULL; + if (idling_boosts_thr_without_issues(bfqd, bfqq) || proc_ref == 0) + goto out; /* next function will take at least one ref */ new_bfqq = bfq_setup_merge(bfqq, stable_merge_bfqq); @@ -2873,6 +2873,11 @@ bfq_setup_stable_merge(struct bfq_data *bfqd, struct bfq_queue *bfqq, new_bfqq_data->stably_merged = true; } } + +out: + /* deschedule stable merge, because done or aborted here */ + bfq_put_stable_ref(stable_merge_bfqq); + return new_bfqq; } @@ -2933,11 +2938,6 @@ bfq_setup_cooperator(struct bfq_data *bfqd, struct bfq_queue *bfqq, struct bfq_queue *stable_merge_bfqq = bfqq_data->stable_merge_bfqq; - /* deschedule stable merge, because done or aborted here */ - bfq_put_stable_ref(stable_merge_bfqq); - - bfqq_data->stable_merge_bfqq = NULL; - return bfq_setup_stable_merge(bfqd, bfqq, stable_merge_bfqq, bfqq_data);