| Message ID | 20230113094410.2907223-1-yukuai3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block, bfq: fix uaf for bfqq in bic_set_bfqq() | expand |
On Fri 13-01-23 17:44:10, Yu Kuai wrote: > After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), > bic->bfqq will be accessed in bic_set_bfqq(), however, in some context > bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed > bic->bfqq. > > Fix the problem by always freeing bfqq after bic_set_bfqq(). > > Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Looks good, thanks for the fix! Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/bfq-cgroup.c | 2 +- > block/bfq-iosched.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index a6e8da5f5cfd..feb13ac25557 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 815b884d6c5a..2ddf831221c4 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -5581,9 +5581,11 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio) > > bfqq = bic_to_bfqq(bic, false, bfq_actuator_index(bfqd, bio)); > if (bfqq) { > - bfq_release_process_ref(bfqd, bfqq); > + struct bfq_queue *old_bfqq = bfqq; > + > bfqq = bfq_get_queue(bfqd, bio, false, bic, true); > bic_set_bfqq(bic, bfqq, false, bfq_actuator_index(bfqd, bio)); > + bfq_release_process_ref(bfqd, old_bfqq); > } > > bfqq = bic_to_bfqq(bic, true, bfq_actuator_index(bfqd, bio)); > -- > 2.31.1 >
On 2023-01-13 10:44, Yu Kuai wrote: > After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), > bic->bfqq will be accessed in bic_set_bfqq(), however, in some context > bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed > bic->bfqq. > > Fix the problem by always freeing bfqq after bic_set_bfqq(). > > Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki-Sjgp3cTcYWE@public.gmane.org> > Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> > --- > block/bfq-cgroup.c | 2 +- > block/bfq-iosched.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index a6e8da5f5cfd..feb13ac25557 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 815b884d6c5a..2ddf831221c4 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -5581,9 +5581,11 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio) > > bfqq = bic_to_bfqq(bic, false, bfq_actuator_index(bfqd, bio)); > if (bfqq) { > - bfq_release_process_ref(bfqd, bfqq); > + struct bfq_queue *old_bfqq = bfqq; > + > bfqq = bfq_get_queue(bfqd, bio, false, bic, true); > bic_set_bfqq(bic, bfqq, false, bfq_actuator_index(bfqd, bio)); > + bfq_release_process_ref(bfqd, old_bfqq); > } > > bfqq = bic_to_bfqq(bic, true, bfq_actuator_index(bfqd, bio)); > Hello, does this condition also affect the current code? I ask since the patch does not apply as bfq_sync_bfqq_move() is only part of the multi-actuator work, which is only in Jens' for-next. Comparing the code sections it seems it should also be necessary for current 6.1/6.2, but I wanted to check. thanks Holger
Hi, 在 2023/01/13 21:38, Holger Hoffstätte 写道: > On 2023-01-13 10:44, Yu Kuai wrote: >> After commit 64dc8c732f5c ("block, bfq: fix possible uaf for >> 'bfqq->bic'"), >> bic->bfqq will be accessed in bic_set_bfqq(), however, in some context >> bic->bfqq will be freed first, and bic_set_bfqq() is called with the >> freed >> bic->bfqq. >> >> Fix the problem by always freeing bfqq after bic_set_bfqq(). >> >> Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") >> Reported-and-tested-by: Shinichiro Kawasaki >> <shinichiro.kawasaki-Sjgp3cTcYWE@public.gmane.org> >> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> >> --- >> block/bfq-cgroup.c | 2 +- >> block/bfq-iosched.c | 4 +++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c >> index a6e8da5f5cfd..feb13ac25557 100644 >> --- a/block/bfq-cgroup.c >> +++ b/block/bfq-cgroup.c >> @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, >> * old cgroup. >> */ >> bfq_put_cooperator(sync_bfqq); >> - bfq_release_process_ref(bfqd, sync_bfqq); >> bic_set_bfqq(bic, NULL, true, act_idx); >> + bfq_release_process_ref(bfqd, sync_bfqq); >> } >> } >> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c >> index 815b884d6c5a..2ddf831221c4 100644 >> --- a/block/bfq-iosched.c >> +++ b/block/bfq-iosched.c >> @@ -5581,9 +5581,11 @@ static void bfq_check_ioprio_change(struct >> bfq_io_cq *bic, struct bio *bio) >> bfqq = bic_to_bfqq(bic, false, bfq_actuator_index(bfqd, bio)); >> if (bfqq) { >> - bfq_release_process_ref(bfqd, bfqq); >> + struct bfq_queue *old_bfqq = bfqq; >> + >> bfqq = bfq_get_queue(bfqd, bio, false, bic, true); >> bic_set_bfqq(bic, bfqq, false, bfq_actuator_index(bfqd, bio)); >> + bfq_release_process_ref(bfqd, old_bfqq); >> } >> bfqq = bic_to_bfqq(bic, true, bfq_actuator_index(bfqd, bio)); >> > > Hello, > > does this condition also affect the current code? I ask since the patch > does not apply > as bfq_sync_bfqq_move() is only part of the multi-actuator work, which > is only in > Jens' for-next. Comparing the code sections it seems it should also be > necessary for > current 6.1/6.2, but I wanted to check. bfq_sync_bfqq_move() already make sure bfq_release_process_ref() is called after bic_set_bfqq(), so the problem this patch tries to fix should not exist here. Thanks, Kuai > > thanks > Holger > > . >
On Jan 13, 2023 / 17:44, Yu Kuai wrote: > After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), > bic->bfqq will be accessed in bic_set_bfqq(), however, in some context > bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed > bic->bfqq. > > Fix the problem by always freeing bfqq after bic_set_bfqq(). > > Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-cgroup.c | 2 +- > block/bfq-iosched.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index a6e8da5f5cfd..feb13ac25557 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } > Yu, thanks for posting this fix, but it can not be applied to v6.2-rc5. The hunk above looks different from the patch I tested. Could you take a look?
Hi, 在 2023/01/24 8:09, Shinichiro Kawasaki 写道: > > Yu, thanks for posting this fix, but it can not be applied to v6.2-rc5. The > hunk above looks different from the patch I tested. Could you take a look? > This patch was rebased with following patch that add a new param for bic_set_bfqq(): 51ec2387623a block, bfq: split sync bfq_queues on a per-actuator basis Thanks, Kuai
Hi, Jens 在 2023/01/13 17:44, Yu Kuai 写道: > After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), > bic->bfqq will be accessed in bic_set_bfqq(), however, in some context > bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed > bic->bfqq. > > Fix the problem by always freeing bfqq after bic_set_bfqq(). > Sorry that I send this patch will wrong email, and you might missed this patch. Can you apply this patch? This patch can't be applied directly to lower version due to Paolo's patchset, I'll send lts patch seperately. Thanks, Kuai > Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
On 1/28/23 6:38 PM, Yu Kuai wrote: > Hi, Jens > > 在 2023/01/13 17:44, Yu Kuai 写道: >> After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), >> bic->bfqq will be accessed in bic_set_bfqq(), however, in some context >> bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed >> bic->bfqq. >> >> Fix the problem by always freeing bfqq after bic_set_bfqq(). >> > > Sorry that I send this patch will wrong email, and you might missed this > patch. > > Can you apply this patch? This patch can't be applied directly to lower > version due to Paolo's patchset, I'll send lts patch seperately. I'm confused... So this patch only applies to the 6.3 branch, yet we need it in 6.2 as far as I can tell. Why isn't it against block-6.2 then?
Hi, 在 2023/01/30 5:51, Jens Axboe 写道: > On 1/28/23 6:38 PM, Yu Kuai wrote: >> Hi, Jens >> >> 在 2023/01/13 17:44, Yu Kuai 写道: >>> After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), >>> bic->bfqq will be accessed in bic_set_bfqq(), however, in some context >>> bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed >>> bic->bfqq. >>> >>> Fix the problem by always freeing bfqq after bic_set_bfqq(). >>> >> >> Sorry that I send this patch will wrong email, and you might missed this >> patch. >> >> Can you apply this patch? This patch can't be applied directly to lower >> version due to Paolo's patchset, I'll send lts patch seperately. > > I'm confused... So this patch only applies to the 6.3 branch, yet we > need it in 6.2 as far as I can tell. Why isn't it against block-6.2 > then? > Ok, I'll send a new patch against block-6.2. Thanks, Kuai
Hi, Jens 在 2023/01/13 17:44, Yu Kuai 写道: > After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), > bic->bfqq will be accessed in bic_set_bfqq(), however, in some context > bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed > bic->bfqq. > > Fix the problem by always freeing bfqq after bic_set_bfqq(). > > Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") > Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > block/bfq-cgroup.c | 2 +- > block/bfq-iosched.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c > index a6e8da5f5cfd..feb13ac25557 100644 > --- a/block/bfq-cgroup.c > +++ b/block/bfq-cgroup.c > @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, > * old cgroup. > */ > bfq_put_cooperator(sync_bfqq); > - bfq_release_process_ref(bfqd, sync_bfqq); > bic_set_bfqq(bic, NULL, true, act_idx); > + bfq_release_process_ref(bfqd, sync_bfqq); > } > } > It seems this change is missed in GIT PULL for-6.3. I'll send a seperate patch to fix this... Thanks, Kuai
On 2023-02-21 08:04, Yu Kuai wrote: > Hi, Jens > > 在 2023/01/13 17:44, Yu Kuai 写道: >> After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), >> bic->bfqq will be accessed in bic_set_bfqq(), however, in some context >> bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed >> bic->bfqq. >> >> Fix the problem by always freeing bfqq after bic_set_bfqq(). >> >> Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") >> Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> block/bfq-cgroup.c | 2 +- >> block/bfq-iosched.c | 4 +++- >> 2 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c >> index a6e8da5f5cfd..feb13ac25557 100644 >> --- a/block/bfq-cgroup.c >> +++ b/block/bfq-cgroup.c >> @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, >> * old cgroup. >> */ >> bfq_put_cooperator(sync_bfqq); >> - bfq_release_process_ref(bfqd, sync_bfqq); >> bic_set_bfqq(bic, NULL, true, act_idx); >> + bfq_release_process_ref(bfqd, sync_bfqq); >> } >> } >> > > It seems this change is missed in GIT PULL for-6.3. I'll send a seperate > patch to fix this... > It was already applied in time for 6.2 as b600de2d7d3a16f9007fad1bdae82a3951a26af2 and also already merged to 6.1-stable. cheers Holger
Hi, 在 2023/02/21 18:19, Holger Hoffstätte 写道: > On 2023-02-21 08:04, Yu Kuai wrote: >> Hi, Jens >> >> 在 2023/01/13 17:44, Yu Kuai 写道: >>> After commit 64dc8c732f5c ("block, bfq: fix possible uaf for >>> 'bfqq->bic'"), >>> bic->bfqq will be accessed in bic_set_bfqq(), however, in some context >>> bic->bfqq will be freed first, and bic_set_bfqq() is called with the >>> freed >>> bic->bfqq. >>> >>> Fix the problem by always freeing bfqq after bic_set_bfqq(). >>> >>> Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") >>> Reported-and-tested-by: Shinichiro Kawasaki >>> <shinichiro.kawasaki@wdc.com> >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> --- >>> block/bfq-cgroup.c | 2 +- >>> block/bfq-iosched.c | 4 +++- >>> 2 files changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c >>> index a6e8da5f5cfd..feb13ac25557 100644 >>> --- a/block/bfq-cgroup.c >>> +++ b/block/bfq-cgroup.c >>> @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data >>> *bfqd, >>> * old cgroup. >>> */ >>> bfq_put_cooperator(sync_bfqq); >>> - bfq_release_process_ref(bfqd, sync_bfqq); >>> bic_set_bfqq(bic, NULL, true, act_idx); >>> + bfq_release_process_ref(bfqd, sync_bfqq); >>> } >>> } >>> >> >> It seems this change is missed in GIT PULL for-6.3. I'll send a seperate >> patch to fix this... >> > > It was already applied in time for 6.2 as > b600de2d7d3a16f9007fad1bdae82a3951a26af2 > and also already merged to 6.1-stable. Yes, 6.2 and 6.1 doesn't have such problem because bfq_sync_bfqq_move() doesn't exist. The problem only exist in master branch currently. Thanks, Kuai > > cheers > Holger > > . >
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index a6e8da5f5cfd..feb13ac25557 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -749,8 +749,8 @@ static void bfq_sync_bfqq_move(struct bfq_data *bfqd, * old cgroup. */ bfq_put_cooperator(sync_bfqq); - bfq_release_process_ref(bfqd, sync_bfqq); bic_set_bfqq(bic, NULL, true, act_idx); + bfq_release_process_ref(bfqd, sync_bfqq); } } diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 815b884d6c5a..2ddf831221c4 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5581,9 +5581,11 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio) bfqq = bic_to_bfqq(bic, false, bfq_actuator_index(bfqd, bio)); if (bfqq) { - bfq_release_process_ref(bfqd, bfqq); + struct bfq_queue *old_bfqq = bfqq; + bfqq = bfq_get_queue(bfqd, bio, false, bic, true); bic_set_bfqq(bic, bfqq, false, bfq_actuator_index(bfqd, bio)); + bfq_release_process_ref(bfqd, old_bfqq); } bfqq = bic_to_bfqq(bic, true, bfq_actuator_index(bfqd, bio));
After commit 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'"), bic->bfqq will be accessed in bic_set_bfqq(), however, in some context bic->bfqq will be freed first, and bic_set_bfqq() is called with the freed bic->bfqq. Fix the problem by always freeing bfqq after bic_set_bfqq(). Fixes: 64dc8c732f5c ("block, bfq: fix possible uaf for 'bfqq->bic'") Reported-and-tested-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- block/bfq-cgroup.c | 2 +- block/bfq-iosched.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)