[PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

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

[PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[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-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));
-- 
2.31.1

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[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.

thanks
Holger

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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
> 
> .
> 

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Shinichiro Kawasaki]

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?

-- 
Shin'ichiro Kawasaki

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Shinichiro Kawasaki]

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

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?

-- 
Shin'ichiro Kawasaki

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu 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>

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu 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-Sjgp3cTcYWE@public.gmane.org>
> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[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?

-- 
Jens Axboe

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu 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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu 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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[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.

cheers
Holger

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

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

Re: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq()

[Yu Kuai]

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
> 
> .
>