Message ID | 20230113094410.2907223-1-yukuai3@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp171638wrt; Fri, 13 Jan 2023 01:30:45 -0800 (PST) X-Google-Smtp-Source: AMrXdXs9ailWYjMJrbJLAXkqNQqdilnt4WZ23JltOT76yRLMcMgo4TkSX0ziCOYvHlrm5MzyHmeG X-Received: by 2002:a05:6402:34c1:b0:49b:7e30:ae4a with SMTP id w1-20020a05640234c100b0049b7e30ae4amr4644360edc.31.1673602245251; Fri, 13 Jan 2023 01:30:45 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673602245; cv=none; d=google.com; s=arc-20160816; b=RYtKlPKHvXw4lA+B3UmZHCVg0IvvJXQkMdR2rADG6pJ3GM3j5wnqNRrmeBIYqrIqIH CYoPVo+HFpJPlK1wi27RIWbtksEajIEWz/PjUaFnIxIGTUiMfNcNk/UXmC6QS9dc2gL2 ravTEku1UGSnjTGtSy2Nbh/sJGOxZlbWOTumS/+qjQSVnAbA0gNgRnVx2zPCqNfEWQFE eqIpkAQ4IOrRdJIdFRFJnBgp9+aEhwtkx+xxMI0I+mm/PkljFhyrVUFVgHanLay6kkr9 WUus71kW2hWk5M2qjXqSXXO7FObAQJQyYHFC/EUWtpa4nHtabF5tefQRhhap5hRtiLzM 06KQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=YlkAPfUn7fD5DChGiRjEAntnbgOmeZJg2pX0dEknSn4=; b=xb05KCzGDtfbZF4m3+T/TkhBg9XPaU9FjD66kLYRRoWkreBJcFMA+pjYKoHlvItk+Z 0yBLEjxM1FQXocT2hW6c5emQJY5heMwBBUpx3boq86LZGqWAy69ck4f7Lgt8gXYEzPTc irsQuT+iTnYKoDxJHXJgbF44ACldma5NpxkYqJF1JRXm8tGhnxko+msGluCQ4KEs/oP6 2M2YOF32X920gNicpQBaUwsCrfAbo6zZmpK0mkecz9Rcx6nycTzJsNPi6FnHzsqC/lTk pWzR3iBhU8AsWSSsf84arQrVLILLRg5P8aghn3M4ed7CGNp7Zy/psL50OQptmMshd1tK JWtA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s6-20020a056402520600b00493952a2f8esi10458086edd.269.2023.01.13.01.30.18; Fri, 13 Jan 2023 01:30:45 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241126AbjAMJ1A (ORCPT <rfc822;callmefire3@gmail.com> + 99 others); Fri, 13 Jan 2023 04:27:00 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53670 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240903AbjAMJZ6 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 13 Jan 2023 04:25:58 -0500 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 39A3213DD0; Fri, 13 Jan 2023 01:19:50 -0800 (PST) Received: from kwepemm600009.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4NtbNL2NLGzqVHW; Fri, 13 Jan 2023 17:14:58 +0800 (CST) Received: from huawei.com (10.175.127.227) by kwepemm600009.china.huawei.com (7.193.23.164) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Fri, 13 Jan 2023 17:19:46 +0800 From: Yu Kuai <yukuai3@huawei.com> To: <jack@suse.cz>, <tj@kernel.org>, <josef@toxicpanda.com>, <axboe@kernel.dk>, <paolo.valente@linaro.org>, <shinichiro.kawasaki@wdc.com>, <yukuai3@huawei.com> CC: <cgroups@vger.kernel.org>, <linux-block@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yukuai1@huaweicloud.com>, <yi.zhang@huawei.com>, <yangerkun@huawei.com> Subject: [PATCH] block, bfq: fix uaf for bfqq in bic_set_bfqq() Date: Fri, 13 Jan 2023 17:44:10 +0800 Message-ID: <20230113094410.2907223-1-yukuai3@huawei.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.127.227] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemm600009.china.huawei.com (7.193.23.164) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754899148089732929?= X-GMAIL-MSGID: =?utf-8?q?1754899148089732929?= |
Series |
block, bfq: fix uaf for bfqq in bic_set_bfqq()
|
|
Commit Message
Yu Kuai
Jan. 13, 2023, 9:44 a.m. UTC
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(-)
Comments
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 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));