On 10/11/24 08:07, Jean Delvare wrote:
Hi all,
I recently got to look into the cpu5wdt driver to backport a security
fix. This driver seems to be in a pretty sorry state.
1* The driver accesses arbitrary I/O ports without ever checking which
hardware it runs on. I have no idea what this "sma cpu5" thing is,
a quick web search did not return anything relevant, only that
sma.de still exists but nothing about "CPU5" specifically. The
Kconfig description is "TBD" (not joking...). I can only imagine
this is a very specific, very old and probably no longer relevant
piece of hardware. The driver comes with zero documentation. I tried
to contact the original author but did not get any answer.
2* There's no explanation of what cpu5wdt_lock is supposed to protect,
but its use throughout the driver is suspiciously inconsistent. In
most places, the lock is held when accessing cpu5wdt_device.running,
ticks, cpu5wdt_device.queue, cpu5wdt_device.timer and
cpu5wdt_device.stop. However there are exceptions to that, with
cpu5wdt_trigger() using cpu5wdt_device.running and ticks without
holding the lock. Likewise, cpu5wdt_exit() is using
cpu5wdt_device.queue, cpu5wdt_device.stop and cpu5wdt_device.timer
without holding the lock, which could race with cpu5wdt_trigger().
cpu5wdt_reset() is also touching ticks without holding the lock. Not
sure how atomic "ticks--" is guaranteed to be on X86, if atomicity
isn't fully guaranteed then this is racy (FWIW ticks was originally
declared volatile, but volatile was removed to silent a warning).
3* The driver doesn't implement WDIOC_SETTIMEOUT. The timeout is set
at module load time as an arbitrary tick count. The default is
10000 and the driver is ticking every (HZ/10+1) jiffies, which leads
to an inaccurate and undocumented 16 minutes 40 seconds timeout,
after which the driver stops pinging the hardware but we have no
idea how long it will take before the hardware watchdog actually
reboots the system. This is clearly incompatible with the
expectations of modern software stacks (systemd, high-availability
setups).
4* I also found a potential integer overflow in cpu5wdt_start, using
WDIOS_ENABLECARD 2^64 times without closing the device would "stop
the watchdog", so the system would not reboot even if the watchdog
process stops writing to the device. Not a serious issue as nobody
sane would do that, but still...
The driver was added in 2003 and there's no evidence that it has any
recent user, all changes seem to be tree-wide, subsystem-wide, or the
result of static code analysis.
Would anyone object to the plain removal of driver cpu5wdt from the
kernel tree?
Ok with me to remove it. Unless someone objects direectly with a good reason,
I'd suggest to submit a patch to remove it and wait for the fallout.
Guenter
