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

[Borislav Petkov]

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