> Il giorno 13 mar 2021, alle ore 15:00, lyl2019@xxxxxxxxxxxxxxxx ha scritto:
>
>
>
>
>> -----原始邮件-----
>> 发件人: "Paolo Valente" <paolo.valente@xxxxxxxxxx>
>> 发送时间: 2021-03-12 22:47:22 (星期五)
>> 收件人: lyl2019@xxxxxxxxxxxxxxxx
>> 抄送: "Jens Axboe" <axboe@xxxxxxxxx>, linux-block@xxxxxxxxxxxxxxx, security@xxxxxxxxxx
>> 主题: Re: [BUG] block/bfq-wf2q: A Use After Free bug in bfq_del_bfqq_busy
>>
>>
>>
>>> Il giorno 10 mar 2021, alle ore 04:15, lyl2019@xxxxxxxxxxxxxxxx ha scritto:
>>>
>>> File: block/bfq-wf2q.c
>>>
>>> There exist a feasible path to trigger a use after free bug in
>>> bfq_del_bfqq_busy, since v4.12-rc1. It could cause denial of service.
>>>
>>
>> Thank you very much for analyzing this.
>>
>>> In the implemention of bfq_del_bfqq_busy,
>>
>> I've checked all invocations of bfq_del_bfqq_busy, comments below.
>>
>>> it calls bfq_deactivate_bfqq()
>>> and use `bfqq` later. Whereas bfq_deactivate_bfqq() could free `bfqq`.
>>>
>>> The trigger path is as follow:
>>> |- bfq_deactivate_bfqq(.., bfqq, true, ..)
>>> |-- entity = &bfqq->entity; // get entity
>>> |-- bfq_deactivate_entity(entity, true, ...); //has a path to free `bfqq`
>>> |-- if (!bfqq->dispatched) // use after free!
>>>
>>>
>>> |- bfq_deactivate_entity(entity, true, ...)
>>> |-- ...
>>> |-- for_each_entity_safe(entity, parent) { // in the first loop,
>>> //entity is the same as before
>>> if (!__bfq_deactivate_entity(entity, true)) {
>>>
>>> |- __bfq_deactivate_entity(entity, true)
>>> |-- ...
>>> |-- if (!ins_into_idle_tree || !bfq_gt(entity->finish, st->vtime))
>>> bfq_forget_entity(st, entity, is_in_service);
>>>
>>> |- bfq_forget_entity(st, entity, is_in_service)
>>> |-- bfqq = bfq_entity_to_bfqq(entity); // recover `bfqq` by entity
>>> |-- if (bfqq && !is_in_service)
>>> bfq_put_queue(bfqq); // free the `bfqq`
>>>
>>
>> For this put to turn into a free, bfqq should have only one ref. But
>> I did not find any invocation of bfq_del_bfqq_busy with bfqq->ref == 1.
>>
>> Did you spot any?
>>
>> Looking forward to your feedback,
>> Paolo
>>
>>> The bug fix needs to add some checks to avoid freeing `bfqq` in the first
>>> loop in __bfq_deactivate_entity(). I can't come out a good patch for it,
>>> so i report it for you.
>>
>
> I the file block/bfq-iosched.c, function bfq_remove_request get bfqq by
> "RQ_BFQQ(rq);". Can we assume bfqq->ref ==1 at this time? After bfq_remove_request
> got the bfqq, there is no ref increase operation before calls bfq_del_bfqq_busy().
>
The scheme is: a ref is taken for a request arrival into a bfq_queue,
in __bfq_insert_request, and then that ref is released on the
completion of that request, in bfq_finish_requeue_request_body. In
this respect, bfq_remove_request is invoked for a request only before
bfq_finish_requeue_request_body is invoked for the same request. So
the bfq_del_bfqq_busy invocation that you highlight should not lead to
a free, because the ref counter should remain always higher than 0.
If you think I'm missing something (which is possible and welcome!),
try to write the sequence of events that leads to the free you
suspect, and to show that the counter actually reaches zero. Recall
that a deactivate should be paired with an activation, and an
activation should always cause the ref counter to be increased.
Looking forward to your feedback,
Paolo
