linux-kernel - Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod

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>] [day] [month] [year] [list]

Message-ID: <CAJht_ENqaJtUwfcXCC2WXNJi+9_HuYX4SrY-QZd19mZrL0EHBg@mail.gmail.com>
Date:   Mon, 28 Dec 2020 02:22:37 -0800
From:   Xie He <xie.he.0141@...il.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Krzysztof Halasa <khc@...waw.pl>
Subject: Re: [PATCH net v2] net: hdlc_ppp: Fix issues when mod_timer is called
 while timer is running

On Mon, Dec 28, 2020 at 1:17 AM Hillf Danton <hdanton@...a.com> wrote:
>
> On Sun, 27 Dec 2020 18:53:39 -0800 Xie He wrote:
> > ppp_cp_event is called directly or indirectly by ppp_rx with "ppp->lock"
> > held. It may call mod_timer to add a new timer. However, at the same time
> > ppp_timer may be already running and waiting for "ppp->lock". In this
> > case, there's no need for ppp_timer to continue running and it can just
> > exit.
>
> Because the timer callback loses the race in acquiring the ppp->lock
> does not mean it should abort.

I think aborting ppp_timer is the correct solution. When mod_timer is
called by ppp_cp_event, which is (directly or indirectly) called by
ppp_rx, this means we received something on the line that makes the
original timer no longer necessary. If the timer is pending, mod_timer
will delete it. If the timer is running and waiting for the lock, I
think it should abort. This way it would appear that the timer hasn't
fired and is deleted before it fires.

> > If we let ppp_timer continue running, it may call add_timer. This causes
> > kernel panic because add_timer can't be called with a timer pending.
>
> Meanwhie we can defuse the peril following add_timer() added in
> e022c2f07ae5, say by replacing add_timer() with mod_timer().

The code path that calls add_timer is for sending keep-alive packets
when operating in the OPENED state. If we have just changed to the
OPENED state in ppp_cp_event, we should wait for the amount of time
set by the (2nd) mod_timer call in ppp_cp_event, before firing the
timer. We shouldn't fire the timer immediately after we change to the
OPENED state. This is not the intention of the (2nd) mod_timer call in
ppp_cp_event. Therefore aborting ppp_timer is the correct solution.