netdev - Re: [PATCH net] can: bcm: switch timer to HRTIMER_MODE_SOFT and remove hrtimer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fc20454f-68ee-21f5-10a4-8100407aad53@huawei.com>
Date:   Tue, 11 Jan 2022 21:02:37 +0800
From:   "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        Greg KH <gregkh@...uxfoundation.org>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <mkl@...gutronix.de>,
        <netdev@...r.kernel.org>, <stable@...r.kernel.org>,
        <linux-can@...r.kernel.org>, <tglx@...utronix.de>,
        <anna-maria@...utronix.de>
Subject: Re: [PATCH net] can: bcm: switch timer to HRTIMER_MODE_SOFT and
 remove hrtimer_tasklet

> On 11.01.22 03:02, Ziyang Xuan (William) wrote:
>>> On Mon, Jan 10, 2022 at 09:23:22PM +0800, Ziyang Xuan wrote:
>>>> From: Thomas Gleixner <tglx@...utronix.de>
>>>>
>>>> [ commit bf74aa86e111aa3b2fbb25db37e3a3fab71b5b68 upstream ]
>>>>
>>>> Stop tx/rx cycle rely on the active state of tasklet and hrtimer
>>>> sequentially in bcm_remove_op(), the op object will be freed if they
>>>> are all unactive. Assume the hrtimer timeout is short, the hrtimer
>>>> cb has been excuted after tasklet conditional judgment which must be
>>>> false after last round tasklet_kill() and before condition
>>>> hrtimer_active(), it is false when execute to hrtimer_active(). Bug
>>>> is triggerd, because the stopping action is end and the op object
>>>> will be freed, but the tasklet is scheduled. The resources of the op
>>>> object will occur UAF bug.
>>>
>>> That is not the changelog text of this commit.  Why modify it?
>>
>> Above statement is the reason why I want to backport the patch to
>> stable tree. Maybe I could give an extra cover-letter to explain
>> the details of the problem, but modify the original changelog. Is it?
>>
> 
> If you backport the bcm HRTIMER_MODE_SOFT implementation to the 4.19 stable tree the problem is not fixed for 4.14, 4.4, etc.

This backport patch does not fit lts 4.4 and 4.9. In addition,
I backport patch to lts for the first time. So I am going to
backport the patch one by one.

> 
> HRTIMER_MODE_SOFT has been introduced in 4.16
> 
> The issue of a race condition at bcm op removal has already been addressed before in commit a06393ed03167 ("can: bcm: fix hrtimer/tasklet termination in bcm op removal").
> 
> -       hrtimer_cancel(&op->timer);
> -       hrtimer_cancel(&op->thrtimer);
> -
> -       if (op->tsklet.func)
> -               tasklet_kill(&op->tsklet);
> +       if (op->tsklet.func) {
> +               while (test_bit(TASKLET_STATE_SCHED, &op->tsklet.state) ||
> +                      test_bit(TASKLET_STATE_RUN, &op->tsklet.state) ||
> +                      hrtimer_active(&op->timer)) {
> +                       hrtimer_cancel(&op->timer);
> +                       tasklet_kill(&op->tsklet);
> +               }
> +       }
> 
> IMO we should better try to improve this fix and enable it for older stable trees than fixing only the 4.19.

The commit bf74aa86e111 can solve the op UAF nicely and enter mainline from v5.4.
I prefer to backport the patch to lower lts, but other sewing and mending,
for example move hrtimer_active() forward.

> 
> Best regards,
> Oliver
> 
> 
> 
>>>
>>>>
>>>> ----------------------------------------------------------------------
>>>>
>>>> This patch switches the timer to HRTIMER_MODE_SOFT, which executed the
>>>> timer callback in softirq context and removes the hrtimer_tasklet.
>>>>
>>>> Reported-by: syzbot+652023d5376450cc8516@...kaller.appspotmail.com
>>
>> This is the public problem reporter. Do I need to move it to cover-letter
>> but here?
>>
>>>> Cc: stable@...r.kernel.org # 4.19
>>
>> I want to backport the patch to linux-4.19.y stable tree. How do I need to
>> modify?
>>
>>>> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
>>>> Signed-off-by: Anna-Maria Gleixner <anna-maria@...utronix.de>
>>>> Acked-by: Oliver Hartkopp <socketcan@...tkopp.net>
>>>> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@...wei.com>
>>>> ---
>>>>   net/can/bcm.c | 156 +++++++++++++++++---------------------------------
>>>>   1 file changed, 52 insertions(+), 104 deletions(-)
>>>
>>> What stable kernel tree(s) are you wanting this backported to?
>>>
>>> thanks,
>>>
>>> greg k-h
>>> .
>>>
>>
>> Thank you for your patient guidance.
>>
> .