From: Paolo Valente <paolo.valente@linaro.org>
To: "zhangwensheng (E)" <zhangwensheng5@huawei.com>
Cc: Jens Axboe <axboe@kernel.dk>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH -next v3] bfq: fix use-after-free in bfq_dispatch_request
Date: Wed, 19 Jan 2022 14:49:46 +0100 [thread overview]
Message-ID: <00FA42CF-DDAC-41E4-88F6-0C0346BFF68F@linaro.org> (raw)
In-Reply-To: <ecbec03d147e441bb5b91a0cae3fb782@huawei.com>
> Il giorno 23 dic 2021, alle ore 03:06, zhangwensheng (E) <zhangwensheng5@huawei.com> ha scritto:
>
> ping...
>
> -----邮件原件-----
> 发件人: zhangwensheng (E)
> 发送时间: 2021年12月20日 10:35
> 收件人: 'Paolo Valente' <paolo.valente@linaro.org>
> 抄送: Jens Axboe <axboe@kernel.dk>; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org
> 主题: 答复: [PATCH -next v3] bfq: fix use-after-free in bfq_dispatch_request
>
> Hi paolo：
> Thanks for your question, it is my negligence.
> I have two ideas for repairing the problem:
>
Hi,
sorry for the delay. I've been thinking of this UAF a little bit
more, and I've realized that in_serv_queue may become a pending
pointer only if in_serv_queue has no rq queued. But, if in_serv_queue
has no rq queued, then idle_timer_disabled will necessarily be false.
And, if idle_timer_disabled is false, then the parameter in_serv_queue
will not be used inside bfq_update_dispatch_stats. So, after your
initial fix, the rest of the code seems already correct, with no
change.
Maybe, if we don't like to pass a pending pointer, what about
something like
...
bfq_update_dispatch_stats(hctx->queue, rq,
idle_timer_disabled ? in_serv_queue : NULL,
idle_timer_disabled);
(the above is just to give express my idea, please write better code
if needed).
Thanks,
Paolo
> 1. use ref++ to avoid the in_serv_queue being released. Patch as follow:
>
> block/bfq-iosched.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index fec18118dc30..70bd280170f9 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5066,6 +5066,7 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> spin_lock_irq(&bfqd->lock);
>
> in_serv_queue = bfqd->in_service_queue;
> + in_serv_queue->ref++; /* aviod in_serv_queue release */
> waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
>
> rq = __bfq_dispatch_request(hctx); @@ -5077,6 +5078,10 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
>
> bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue,
> idle_timer_disabled);
> + /* resume in_serv_queue */
> + spin_lock_irq(&bfqd->lock);
> + bfq_put_queue(in_serv_queue);
> + spin_unlock_irq(&bfqd->lock);
>
> return rq;
> }
>
> 2. add new changes to previous, taking out bfqq_group(in_serv_queue) from bfq_update_dispatch_stats. Patch as follow:
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -5007,7 +5007,7 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) #ifdef CONFIG_BFQ_CGROUP_DEBUG static void bfq_update_dispatch_stats(struct request_queue *q,
> struct request *rq,
> - struct bfq_queue *in_serv_queue,
> + struct bfq_group *bfqg_in_serv,
> bool idle_timer_disabled) {
> struct bfq_queue *bfqq = rq ? RQ_BFQQ(rq) : NULL; @@ -5039,7 +5039,7 @@ static void bfq_update_dispatch_stats(struct request_queue *q,
> * therefore guaranteed to exist because of the above
> * arguments.
> */
> - bfqg_stats_update_idle_time(bfqq_group(in_serv_queue));
> + bfqg_stats_update_idle_time(bfqg_in_serv);
> if (bfqq) {
> struct bfq_group *bfqg = bfqq_group(bfqq);
>
> @@ -5052,7 +5052,7 @@ static void bfq_update_dispatch_stats(struct request_queue *q, #else static inline void bfq_update_dispatch_stats(struct request_queue *q,
> struct request *rq,
> - struct bfq_queue *in_serv_queue,
> + struct bfq_group
> + *bfqg_in_serv,
> bool idle_timer_disabled) {} #endif /* CONFIG_BFQ_CGROUP_DEBUG */
>
> @@ -5062,20 +5062,23 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> struct request *rq;
> struct bfq_queue *in_serv_queue;
> bool waiting_rq, idle_timer_disabled;
> + struct bfq_group *bfqg_in_serv;
>
> spin_lock_irq(&bfqd->lock);
>
> in_serv_queue = bfqd->in_service_queue;
> waiting_rq = in_serv_queue && bfq_bfqq_wait_request(in_serv_queue);
> + bfqg_in_serv = bfqq_group(in_serv_queue);
>
> rq = __bfq_dispatch_request(hctx);
>
> - idle_timer_disabled =
> - waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
> -
> + if (in_serv_queue == bfqd->in_service_queue) {
> + idle_timer_disabled =
> + waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
> + }
> spin_unlock_irq(&bfqd->lock);
>
> - bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue,
> + bfq_update_dispatch_stats(hctx->queue, rq, bfqg_in_serv,
> idle_timer_disabled);
>
> return rq;
>
> what do you think?
>
> Thanks
> Zhang Wensheng
> -----邮件原件-----
> 发件人: Paolo Valente [mailto:paolo.valente@linaro.org]
> 发送时间: 2021年12月17日 0:28
> 收件人: zhangwensheng (E) <zhangwensheng5@huawei.com>
> 抄送: Jens Axboe <axboe@kernel.dk>; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org
> 主题: Re: [PATCH -next v3] bfq: fix use-after-free in bfq_dispatch_request
>
>
>
>> Il giorno 16 dic 2021, alle ore 13:21, Zhang Wensheng <zhangwensheng5@huawei.com> ha scritto:
>>
>> KASAN reports a use-after-free report when doing normal scsi-mq test
>>
>> [69832.239032]
>> ==================================================================
>> [69832.241810] BUG: KASAN: use-after-free in
>> bfq_dispatch_request+0x1045/0x44b0
>> [69832.243267] Read of size 8 at addr ffff88802622ba88 by task
>> kworker/3:1H/155 [69832.244656] [69832.245007] CPU: 3 PID: 155 Comm:
>> kworker/3:1H Not tainted 5.10.0-10295-g576c6382529e #8 [69832.246626]
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
>> rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 [69832.249069]
>> Workqueue: kblockd blk_mq_run_work_fn [69832.250022] Call Trace:
>> [69832.250541] dump_stack+0x9b/0xce
>> [69832.251232] ? bfq_dispatch_request+0x1045/0x44b0
>> [69832.252243] print_address_description.constprop.6+0x3e/0x60
>> [69832.253381] ? __cpuidle_text_end+0x5/0x5 [69832.254211] ?
>> vprintk_func+0x6b/0x120 [69832.254994] ?
>> bfq_dispatch_request+0x1045/0x44b0
>> [69832.255952] ? bfq_dispatch_request+0x1045/0x44b0
>> [69832.256914] kasan_report.cold.9+0x22/0x3a [69832.257753] ?
>> bfq_dispatch_request+0x1045/0x44b0
>> [69832.258755] check_memory_region+0x1c1/0x1e0 [69832.260248]
>> bfq_dispatch_request+0x1045/0x44b0
>> [69832.261181] ? bfq_bfqq_expire+0x2440/0x2440 [69832.262032] ?
>> blk_mq_delay_run_hw_queues+0xf9/0x170
>> [69832.263022] __blk_mq_do_dispatch_sched+0x52f/0x830
>> [69832.264011] ? blk_mq_sched_request_inserted+0x100/0x100
>> [69832.265101] __blk_mq_sched_dispatch_requests+0x398/0x4f0
>> [69832.266206] ? blk_mq_do_dispatch_ctx+0x570/0x570
>> [69832.267147] ? __switch_to+0x5f4/0xee0 [69832.267898]
>> blk_mq_sched_dispatch_requests+0xdf/0x140
>> [69832.268946] __blk_mq_run_hw_queue+0xc0/0x270 [69832.269840]
>> blk_mq_run_work_fn+0x51/0x60 [69832.278170]
>> process_one_work+0x6d4/0xfe0 [69832.278984] worker_thread+0x91/0xc80
>> [69832.279726] ? __kthread_parkme+0xb0/0x110 [69832.280554] ?
>> process_one_work+0xfe0/0xfe0 [69832.281414] kthread+0x32d/0x3f0
>> [69832.282082] ? kthread_park+0x170/0x170 [69832.282849]
>> ret_from_fork+0x1f/0x30 [69832.283573] [69832.283886] Allocated by
>> task 7725:
>> [69832.284599] kasan_save_stack+0x19/0x40 [69832.285385]
>> __kasan_kmalloc.constprop.2+0xc1/0xd0
>> [69832.286350] kmem_cache_alloc_node+0x13f/0x460 [69832.287237]
>> bfq_get_queue+0x3d4/0x1140 [69832.287993]
>> bfq_get_bfqq_handle_split+0x103/0x510
>> [69832.289015] bfq_init_rq+0x337/0x2d50 [69832.289749]
>> bfq_insert_requests+0x304/0x4e10 [69832.290634]
>> blk_mq_sched_insert_requests+0x13e/0x390
>> [69832.291629] blk_mq_flush_plug_list+0x4b4/0x760
>> [69832.292538] blk_flush_plug_list+0x2c5/0x480 [69832.293392]
>> io_schedule_prepare+0xb2/0xd0 [69832.294209]
>> io_schedule_timeout+0x13/0x80 [69832.295014]
>> wait_for_common_io.constprop.1+0x13c/0x270
>> [69832.296137] submit_bio_wait+0x103/0x1a0 [69832.296932]
>> blkdev_issue_discard+0xe6/0x160 [69832.297794]
>> blk_ioctl_discard+0x219/0x290 [69832.298614]
>> blkdev_common_ioctl+0x50a/0x1750 [69832.304715]
>> blkdev_ioctl+0x470/0x600 [69832.305474] block_ioctl+0xde/0x120
>> [69832.306232] vfs_ioctl+0x6c/0xc0 [69832.306877]
>> __se_sys_ioctl+0x90/0xa0 [69832.307629] do_syscall_64+0x2d/0x40
>> [69832.308362] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [69832.309382]
>> [69832.309701] Freed by task 155:
>> [69832.310328] kasan_save_stack+0x19/0x40 [69832.311121]
>> kasan_set_track+0x1c/0x30 [69832.311868]
>> kasan_set_free_info+0x1b/0x30 [69832.312699]
>> __kasan_slab_free+0x111/0x160 [69832.313524]
>> kmem_cache_free+0x94/0x460 [69832.314367] bfq_put_queue+0x582/0x940
>> [69832.315112] __bfq_bfqd_reset_in_service+0x166/0x1d0
>> [69832.317275] bfq_bfqq_expire+0xb27/0x2440 [69832.318084]
>> bfq_dispatch_request+0x697/0x44b0 [69832.318991]
>> __blk_mq_do_dispatch_sched+0x52f/0x830
>> [69832.319984] __blk_mq_sched_dispatch_requests+0x398/0x4f0
>> [69832.321087] blk_mq_sched_dispatch_requests+0xdf/0x140
>> [69832.322225] __blk_mq_run_hw_queue+0xc0/0x270 [69832.323114]
>> blk_mq_run_work_fn+0x51/0x60 [69832.323942]
>> process_one_work+0x6d4/0xfe0 [69832.324772] worker_thread+0x91/0xc80
>> [69832.325518] kthread+0x32d/0x3f0 [69832.326205]
>> ret_from_fork+0x1f/0x30 [69832.326932] [69832.338297] The buggy
>> address belongs to the object at ffff88802622b968 [69832.338297]
>> which belongs to the cache bfq_queue of size 512 [69832.340766] The
>> buggy address is located 288 bytes inside of [69832.340766] 512-byte
>> region [ffff88802622b968, ffff88802622bb68) [69832.343091] The buggy
>> address belongs to the page:
>> [69832.344097] page:ffffea0000988a00 refcount:1 mapcount:0
>> mapping:0000000000000000 index:0xffff88802622a528 pfn:0x26228
>> [69832.346214] head:ffffea0000988a00 order:2 compound_mapcount:0
>> compound_pincount:0 [69832.347719] flags: 0x1fffff80010200(slab|head)
>> [69832.348625] raw: 001fffff80010200 ffffea0000dbac08 ffff888017a57650
>> ffff8880179fe840 [69832.354972] raw: ffff88802622a528 0000000000120008
>> 00000001ffffffff 0000000000000000 [69832.356547] page dumped because:
>> kasan: bad access detected [69832.357652] [69832.357970] Memory state around the buggy address:
>> [69832.358926] ffff88802622b980: fb fb fb fb fb fb fb fb fb fb fb fb
>> fb fb fb fb [69832.360358] ffff88802622ba00: fb fb fb fb fb fb fb fb
>> fb fb fb fb fb fb fb fb [69832.361810] >ffff88802622ba80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [69832.363273] ^
>> [69832.363975] ffff88802622bb00: fb fb fb fb fb fb fb fb fb fb fb fb
>> fb fc fc fc [69832.375960] ffff88802622bb80: fc fc fc fc fc fc fc fc
>> fc fc fc fc fc fc fc fc [69832.377405]
>> ==================================================================
>>
>> In bfq_dispatch_requestfunction, it may have function call:
>>
>> bfq_dispatch_request
>> __bfq_dispatch_request
>> bfq_select_queue
>> bfq_bfqq_expire
>> __bfq_bfqd_reset_in_service
>> bfq_put_queue
>> kmem_cache_free
>> In this function call, in_serv_queue has beed expired and meet the
>> conditions to free. In the function bfq_dispatch_request, the address
>> of in_serv_queue pointing to has been released. For getting the value
>> of idle_timer_disabled, it will get flags value from the address which
>> in_serv_queue pointing to, then the problem of use-after-free happens;
>>
>> Fix the problem by check in_serv_queue == bfqd->in_service_queue, to
>> get the value of idle_timer_disabled if in_serve_queue is equel to
>> bfqd->in_service_queue. If the space of in_serv_queue pointing has
>> been released, this judge will aviod use-after-free problem.
>> And if in_serv_queue may be expired but it still exists, this judge
>> may have little effects on the function bfqg_stats_update_idle_time in
>> bfq_update_dispatch_stats.
>>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Zhang Wensheng <zhangwensheng5@huawei.com>
>> ---
>> block/bfq-iosched.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index
>> fec18118dc30..97533634b99e 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -5061,7 +5061,7 @@ static struct request *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
>> struct bfq_data *bfqd = hctx->queue->elevator->elevator_data;
>> struct request *rq;
>> struct bfq_queue *in_serv_queue;
>> - bool waiting_rq, idle_timer_disabled;
>> + bool waiting_rq, idle_timer_disabled = false;
>>
>> spin_lock_irq(&bfqd->lock);
>>
>> @@ -5070,9 +5070,10 @@ static struct request
>> *bfq_dispatch_request(struct blk_mq_hw_ctx *hctx)
>>
>> rq = __bfq_dispatch_request(hctx);
>>
>> - idle_timer_disabled =
>> - waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
>> -
>> + if (in_serv_queue == bfqd->in_service_queue) {
>> + idle_timer_disabled =
>> + waiting_rq && !bfq_bfqq_wait_request(in_serv_queue);
>> + }
>
> Good catch!
>
>> spin_unlock_irq(&bfqd->lock);
>>
>> bfq_update_dispatch_stats(hctx->queue, rq, in_serv_queue,
>
> Yet, what about the above use of in_serv_queue then?
>
> Thanks,
> Paolo
>
>> --
>> 2.31.1
>>
>
next prev parent reply other threads:[~2022-01-19 13:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-16 12:21 [PATCH -next v3] bfq: fix use-after-free in bfq_dispatch_request Zhang Wensheng
2021-12-16 16:27 ` Paolo Valente
2021-12-20 2:35 ` 答复: " zhangwensheng (E)
2021-12-23 2:06 ` zhangwensheng (E)
2022-01-19 13:49 ` Paolo Valente [this message]
2022-01-04 8:58 ` zhangwensheng (E)
2022-01-19 2:39 ` zhangwensheng (E)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=00FA42CF-DDAC-41E4-88F6-0C0346BFF68F@linaro.org \
--to=paolo.valente@linaro.org \
--cc=axboe@kernel.dk \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=zhangwensheng5@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.