I am Seunghun Han and a senior security researcher at National Security Research Institute of South Korea.
I found a security issue which can make kernel panic in userspace. After analyzing the issue carefully, I found that MCE driver in the kernel has a problem which can be occurred in SMP environment. The check_interval file in /sys/devices/system/machinecheck/machinecheck<cpu number> directory is a global timer value for MCE polling. If it is changed by one CPU, MCE driver in kernel calls mce_restart() function in store_int_with_restart() function and broadcasts the event to other CPUs to delete and restart MCE polling timer. The __mcheck_cpu_init_timer() function which is called by mce_restart() function initializes the mce_timer variable, and the "lock" in mce_timer is also reinitialized. If more than one CPU write a specific value to check_interval file concurrently, one can initialize the "lock" in mce_timer while the others are handling "lock" in mce_timer. This problem causes some synchronization errors such as kernel panic and kernel hang. Other functions such as set_ignore_ce(), set_cmci_disabled(), and mce_enable_ce() also have synchronization problems. It could be a security problem because the attacker could make kernel panic by writing a value to the check_interval file in userspace, and it could be used for Denial-of-Service (DoS) attack. To fix this problem, I added a mce_sysfs_mutex to serialize requests for timer and sysfs functions. Signed-off-by: Seunghun Han <kkama...@gmail.com> --- Changes since v2: add a mutex to sysfs functions according to review result. Changes since v1: add mce_sysfs_mutex according to review result. arch/x86/kernel/cpu/mcheck/mce.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c index 706584681a4c..243f46a40efb 100644 --- a/arch/x86/kernel/cpu/mcheck/mce.c +++ b/arch/x86/kernel/cpu/mcheck/mce.c @@ -55,6 +55,7 @@ #include "mce-internal.h" static DEFINE_MUTEX(mce_log_mutex); +static DEFINE_MUTEX(mce_sysfs_mutex); #define CREATE_TRACE_POINTS #include <trace/events/mce.h> @@ -2045,8 +2046,11 @@ static void mce_enable_ce(void *all) return; cmci_reenable(); cmci_recheck(); - if (all) + if (all) { + mutex_lock(&mce_sysfs_mutex); __mcheck_cpu_init_timer(); + mutex_unlock(&mce_sysfs_mutex); + } } static struct bus_type mce_subsys = { @@ -2090,6 +2094,7 @@ static ssize_t set_ignore_ce(struct device *s, if (kstrtou64(buf, 0, &new) < 0) return -EINVAL; + mutex_lock(&mce_sysfs_mutex); if (mca_cfg.ignore_ce ^ !!new) { if (new) { /* disable ce features */ @@ -2102,6 +2107,8 @@ static ssize_t set_ignore_ce(struct device *s, on_each_cpu(mce_enable_ce, (void *)1, 1); } } + mutex_unlock(&mce_sysfs_mutex); + return size; } @@ -2114,6 +2121,7 @@ static ssize_t set_cmci_disabled(struct device *s, if (kstrtou64(buf, 0, &new) < 0) return -EINVAL; + mutex_lock(&mce_sysfs_mutex); if (mca_cfg.cmci_disabled ^ !!new) { if (new) { /* disable cmci */ @@ -2125,6 +2133,8 @@ static ssize_t set_cmci_disabled(struct device *s, on_each_cpu(mce_enable_ce, NULL, 1); } } + mutex_unlock(&mce_sysfs_mutex); + return size; } @@ -2132,8 +2142,16 @@ static ssize_t store_int_with_restart(struct device *s, struct device_attribute *attr, const char *buf, size_t size) { + unsigned long old_check_interval = check_interval; ssize_t ret = device_store_int(s, attr, buf, size); + + if (check_interval == old_check_interval) + return ret; + + mutex_lock(&mce_sysfs_mutex); mce_restart(); + mutex_unlock(&mce_sysfs_mutex); + return ret; } -- 2.16.2