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
