Question about cpu5wdt del_timer fix

Question about cpu5wdt del_timer fix

[Jean Delvare]

Hi Zhou,

I have been asked to backport kernel commit 573601521277 ("watchdog:
cpu5wdt.c: Fix use-after-free bug caused by cpu5wdt_trigger") because
it was assigned a CVE number (CVE-2024-38630).

I was about to just backport the commit as it is pretty simple, however
out of curiosity I looked at the code and I must say I just can't see
which bug is being fixed.

The commit description says that there was a race condition if
cpu5wdt_trigger() was still running after del_timer() when
release_region() is being called as part of cpu5wdt_exit().

The way I read the original code (before your commit):

* In cpu5wdt_exit(), del_timer() is called after
  wait_for_completion(&cpu5wdt_device.stop).

* The completion happens at only one point in the driver, that's in
  cpu5wdt_trigger(), and that's an alternative to rearming the timer.
  Therefore we are guaranteed that no timer is armed when we reach
  del_timer() in cpu5wdt_exit().

* Also this happens *after* the outb() to the I/O region, and I don't
  think the compiler is allowed to reorder that. So even if
  cpu5wdt_trigger() still has to finish after the completion, it can't
  race with release_region().

So I do not think that replacing del_timer() with timer_shutdown_sync()
was needed. The only point of switching to timer_shutdown_sync(), as I
understand it, would be to get rid of the completion to simplify the
code a bit.

Am I missing anything?

-- 
Jean Delvare
SUSE L3 Support

Re: Question about cpu5wdt del_timer fix

[Jean Delvare]

On Wed, 3 Jul 2024 16:39:58 +0200, Jean Delvare wrote:
> I have been asked to backport kernel commit 573601521277 ("watchdog:
> cpu5wdt.c: Fix use-after-free bug caused by cpu5wdt_trigger") because
> it was assigned a CVE number (CVE-2024-38630).
> 
> I was about to just backport the commit as it is pretty simple, however
> out of curiosity I looked at the code and I must say I just can't see
> which bug is being fixed.
> 
> The commit description says that there was a race condition if
> cpu5wdt_trigger() was still running after del_timer() when
> release_region() is being called as part of cpu5wdt_exit().
> 
> The way I read the original code (before your commit):
> 
> * In cpu5wdt_exit(), del_timer() is called after
>   wait_for_completion(&cpu5wdt_device.stop).
> 
> * The completion happens at only one point in the driver, that's in
>   cpu5wdt_trigger(), and that's an alternative to rearming the timer.
>   Therefore we are guaranteed that no timer is armed when we reach
>   del_timer() in cpu5wdt_exit().
> 
> * Also this happens *after* the outb() to the I/O region, and I don't
>   think the compiler is allowed to reorder that. So even if
>   cpu5wdt_trigger() still has to finish after the completion, it can't
>   race with release_region().
> 
> So I do not think that replacing del_timer() with timer_shutdown_sync()
> was needed. The only point of switching to timer_shutdown_sync(), as I
> understand it, would be to get rid of the completion to simplify the
> code a bit.
> 
> Am I missing anything?

Actually I did. One week of vacation gave me time to think about this
some more and I do believe there was actually a race condition, but not
the one described in commit 573601521277.

The race condition I am seeing (before this commit) is that
cpu5wdt_exit() could complete, and thus the module be unloaded and
cpu5wdt_trigger() be removed from memory, while the end of
cpu5wdt_trigger() is still being executed (just one instruction
actually, spin_unlock(&cpu5wdt_lock)).

So the fix was needed after all and CVE-2024-38630 is real, even though
I believe its description is incorrect.

I also believe that timer_shutdown_sync() was not needed as driver
cpu5wdt doesn't involve any workqueue, so timer_delete_sync() would
have been sufficient.

-- 
Jean Delvare
SUSE L3 Support