Re: [PATCH 1/2] x86: Prevent userspace MSR access from dominating the console

From: Borislav Petkov
Date: Wed Aug 19 2020 - 10:50:32 EST


On Mon, Aug 17, 2020 at 04:24:29PM +0100, Chris Down wrote:
> 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(!).
>
> This change makes things much less aggressive, while still achieving the

s/This change makes/Make/

> 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 us 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, this patch keeps the intent of the original patch, while

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> mitigating its sometimes heavy effects on kmsg composition.
>
> Signed-off-by: Chris Down <chris@xxxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> ---
> arch/x86/kernel/msr.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index 49dcfb85e773..3babb0e58b0e 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -80,18 +80,27 @@ static ssize_t msr_read(struct file *file, char __user *buf,
>
> static int filter_write(u32 reg)
> {
> + /*
> + * Banging MSRs usually happens all at once, and can easily saturate

Yeah we have a lot of non-native speakers so "Banging" might be
confusing - please formulate less colloquial.

> + * kmsg. Only allow 1 MSR message every 30 seconds.
> + *
> + * We could be smarter here and do it per-MSR, or whatever, but it would

Please avoid the "we" in text as it is ambiguous - try formulating
passively like your commit message.

> + * certainly be more complex, and this is enough at least to stop
> + * completely 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 (reg == MSR_IA32_ENERGY_PERF_BIAS)
> + if (!__ratelimit(&fw_rs) || reg == MSR_IA32_ENERGY_PERF_BIAS)
> return 0;

Let's keep those two tests separate:

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@xxxxxxxxxx\n",
> - reg, current->comm);
> + pr_err("Write to unrecognized MSR 0x%x by %s\n"
> + "Please report to x86@xxxxxxxxxx\n", reg, current->comm);
>
> return 0;
> }
> --
> 2.28.0
>

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette