Ok, i think it is a false positive now.
Thanks for your time and patient answer.
> -----原始邮件-----
> 发件人: "Paolo Valente" <paolo.valente@xxxxxxxxxx>
> 发送时间: 2021-03-16 15:55:44 (星期二)
> 收件人: 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 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
>
