Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Oct 2022 09:39:46 -0700
From:   Sultan Alsawaf <sultan@kerneltoast.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
        Dominik Brodowski <linux@dominikbrodowski.net>
Subject: Re: [PATCH 2/2] random: spread out jitter callback to different CPUs
Message-ID: <Yz8E0lSmsMMB6KeO@sultan-box.localdomain>
References: <20220930231050.749824-1-Jason@zx2c4.com>
 <20220930231050.749824-2-Jason@zx2c4.com>
 <YzgGmh6EQtWzO4HV@zx2c4.com>
 <Yz2+UsgVGRSm+o7W@linutronix.de>
 <Yz3yQzaNUcdIuUMX@zx2c4.com>
 <Yz55w4gNtZn8JzmG@linutronix.de>
 <Yz7JXEaTFWa1VLKJ@zx2c4.com>
 <Yz7M5zJmzKSk/LYH@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Yz7M5zJmzKSk/LYH@linutronix.de>
Precedence: bulk

Hi Sebastian,

On Thu, Oct 06, 2022 at 02:41:11PM +0200, Sebastian Andrzej Siewior wrote:
> On 2022-10-06 06:26:04 [-0600], Jason A. Donenfeld wrote:
> > e) del_timer_sync() on line 5 is called, and its `base->running_timer !=
> >    timer` check is false, because of step (c).
> 
> If `base->running_timer != timer` then the timer ('s callback) is not
> currently active/ running. Therefore it can be removed from the timer
> bucket (in case it is pending in the future).
> If `base->running_timer == timer` then the timer ('s callback) is
> currently active. del_timer_sync() will loop in cpu_relax() until the
> callback finished. And then try again.
> > f) `stack.timer` gets freed / goes out of scope.
> > 
> > g) The callback scheduled from step (b) runs, and we have a UaF.
> > 
> > That's, anyway, what I understand Sultan to have pointed out to me. In
> > looking at this closely, though, to write this email, I noticed that
> > add_timer_on() does set TIMER_MIGRATING, which lock_timer_base() spins
> > on. So actually, maybe this scenario should be accounted for? Sultan, do
> > you care to comment here?
> 
> During TIMER_MIGRATING the del_timer_sync() caller will spin until the
> condition is over. So it can remove the timer from the right bucket and
> check if it is active vs the right bucket.

My concern stems from the design of add_timer_on(). Specifically, add_timer_on()
expects the timer to not already be pending or running.

Because of this, add_timer_on() doesn't check `base->running_timer` and doesn't
wait for the timer to finish running, because it expects the timer to be
completely idle. Giving add_timer_on() a timer which is already running is a
bug, as made clear by the `BUG_ON(timer_pending(timer) || !timer->function);`.

But since a timer is marked as not-pending prior to when it runs, add_timer_on()
can't detect if the timer is actively running; the above BUG_ON() won't be
tripped. So the UaF scenario I forsee is that doing this:
    add_timer_on(timer, 0);
    // timer is actively running on CPU0, timer is no longer pending
    add_timer_on(timer, 1); // changes timer base, won't wait for timer to stop
    del_timer_sync(timer); // only checks CPU1 timer base for the running timer

may result in the del_timer_sync() not waiting for the timer function to finish
running on CPU0 from the `add_timer_on(timer, 0);`, since add_timer_on() won't
wait for the timer function to finish running before changing the timer base.
And since Jason's timer is declared on the stack, his timer callback function
would dereference `stack.timer` after it's been freed.

> Sebastian

Sultan