From 1f35c9c0ce3888405fc813afedaff79de433cf27 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Fri, 21 Aug 2020 13:10:24 +0100 Subject: x86/msr: Prevent userspace MSR access from dominating the console Applications which manipulate MSRs from userspace often do so infrequently, and all at once. As such, the default printk ratelimit architecture supplied by pr_err_ratelimited() doesn't do enough to prevent kmsg becoming completely overwhelmed with their messages and pushing other salient information out of the circular buffer. In one case, I saw over 80% of kmsg being filled with these messages, and the default kmsg buffer being completely filled less than 5 minutes after boot(!). Make things much less aggressive, while still achieving the original goal of fiter_write(). Operators will still get warnings that MSRs are being manipulated from userspace, but they won't have other also potentially useful messages pushed out of the kmsg buffer. Of course, one can boot with `allow_writes=1` to avoid these messages at all, but that then has the downfall that one doesn't get _any_ notification at all about these problems in the first place, and so is much less likely to forget to fix it. One might rather it was less binary: it was still logged, just less often, so that application developers _do_ have the incentive to improve their current methods, without the kernel having to push other useful stuff out of the kmsg buffer. This one example isn't the point, of course: I'm sure there are plenty of other non-ideal-but-pragmatic cases where people are writing to MSRs from userspace right now, and it will take time for those people to find other solutions. Overall, keep the intent of the original patch, while mitigating its sometimes heavy effects on kmsg composition. [ bp: Massage a bit. ] Signed-off-by: Chris Down Signed-off-by: Borislav Petkov Link: https://lkml.kernel.org/r/563994ef132ce6cffd28fc659254ca37d032b5ef.1598011595.git.chris@chrisdown.name --- arch/x86/kernel/msr.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'arch') diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c index 49dcfb85e773..b03001dfc613 100644 --- a/arch/x86/kernel/msr.c +++ b/arch/x86/kernel/msr.c @@ -80,18 +80,30 @@ static ssize_t msr_read(struct file *file, char __user *buf, static int filter_write(u32 reg) { + /* + * MSRs writes usually happen all at once, and can easily saturate kmsg. + * Only allow one message every 30 seconds. + * + * It's possible to be smarter here and do it (for example) per-MSR, but + * it would certainly be more complex, and this is enough at least to + * avoid saturating the ring buffer. + */ + static DEFINE_RATELIMIT_STATE(fw_rs, 30 * HZ, 1); + switch (allow_writes) { case MSR_WRITES_ON: return 0; case MSR_WRITES_OFF: return -EPERM; default: break; } + if (!__ratelimit(&fw_rs)) + return 0; + if (reg == MSR_IA32_ENERGY_PERF_BIAS) return 0; - pr_err_ratelimited("Write to unrecognized MSR 0x%x by %s\n" - "Please report to x86@kernel.org\n", - reg, current->comm); + pr_err("Write to unrecognized MSR 0x%x by %s\n" + "Please report to x86@kernel.org\n", reg, current->comm); return 0; } -- cgit v1.2.3 From c31feed8461fb8648075ba9b53d9e527d530972f Mon Sep 17 00:00:00 2001 From: Chris Down Date: Fri, 21 Aug 2020 13:10:35 +0100 Subject: x86/msr: Make source of unrecognised MSR writes unambiguous In many cases, task_struct.comm isn't enough to distinguish the offender, since for interpreted languages it's likely just going to be "python3" or whatever. Add the pid to make it unambiguous. [ bp: Make the printk string a single line for easier grepping. ] Signed-off-by: Chris Down Signed-off-by: Borislav Petkov Link: https://lkml.kernel.org/r/6f6fbd0ee6c99bc5e47910db700a6642159db01b.1598011595.git.chris@chrisdown.name --- arch/x86/kernel/msr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'arch') diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c index b03001dfc613..c0d409810658 100644 --- a/arch/x86/kernel/msr.c +++ b/arch/x86/kernel/msr.c @@ -102,8 +102,8 @@ static int filter_write(u32 reg) if (reg == MSR_IA32_ENERGY_PERF_BIAS) return 0; - pr_err("Write to unrecognized MSR 0x%x by %s\n" - "Please report to x86@kernel.org\n", reg, current->comm); + pr_err("Write to unrecognized MSR 0x%x by %s (pid: %d). Please report to x86@kernel.org.\n", + reg, current->comm, current->pid); return 0; } -- cgit v1.2.3 From f94c91f7ba3ba7de2bc8aa31be28e1abb22f849e Mon Sep 17 00:00:00 2001 From: Libing Zhou Date: Thu, 20 Aug 2020 10:56:41 +0800 Subject: x86/nmi: Fix nmi_handle() duration miscalculation When nmi_check_duration() is checking the time an NMI handler took to execute, the whole_msecs value used should be read from the @duration argument, not from the ->max_duration, the latter being used to store the current maximal duration. [ bp: Rewrite commit message. ] Fixes: 248ed51048c4 ("x86/nmi: Remove irq_work from the long duration NMI handler") Suggested-by: Peter Zijlstra (Intel) Signed-off-by: Libing Zhou Signed-off-by: Borislav Petkov Cc: Changbin Du Link: https://lkml.kernel.org/r/20200820025641.44075-1-libing.zhou@nokia-sbell.com --- arch/x86/kernel/nmi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'arch') diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c index 4fc9954a9560..47381666d6a5 100644 --- a/arch/x86/kernel/nmi.c +++ b/arch/x86/kernel/nmi.c @@ -102,7 +102,6 @@ fs_initcall(nmi_warning_debugfs); static void nmi_check_duration(struct nmiaction *action, u64 duration) { - u64 whole_msecs = READ_ONCE(action->max_duration); int remainder_ns, decimal_msecs; if (duration < nmi_longest_ns || duration < action->max_duration) @@ -110,12 +109,12 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration) action->max_duration = duration; - remainder_ns = do_div(whole_msecs, (1000 * 1000)); + remainder_ns = do_div(duration, (1000 * 1000)); decimal_msecs = remainder_ns / 1000; printk_ratelimited(KERN_INFO "INFO: NMI handler (%ps) took too long to run: %lld.%03d msecs\n", - action->handler, whole_msecs, decimal_msecs); + action->handler, duration, decimal_msecs); } static int nmi_handle(unsigned int type, struct pt_regs *regs) -- cgit v1.2.3