[PATCH 0/2] a couple of userspace MSR filtering improvements

[PATCH 0/2] a couple of userspace MSR filtering improvements

[Chris Down]

As discussed in Message-ID <20200714165621.GA3622@nazgul.tnic>.

We all agree that userspace MSR twiddling is non-ideal, but we should be
a bit cautious of taking up too much kmsg buffer if applications do
repeated writes. `allow_writes=1` is possible, but is non-ideal (see
patch 1 changelog).

Also added pid information to the message, since it makes identification
of the source (more or less) unambiguous.

Chris Down (2):
  x86: Prevent userspace MSR access from dominating the console
  x86: Make source of unrecognised MSR writes unambiguous

 arch/x86/kernel/msr.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.28.0

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

[Chris Down]

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
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
mitigating its sometimes heavy effects on kmsg composition.

Signed-off-by: Chris Down <chris@chrisdown.name>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jakub Kicinski <kuba@kernel.org>
---
 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
+	 * kmsg. Only allow 1 MSR message every 30 seconds.
+	 *
+	 * We could be smarter here and do it per-MSR, or whatever, but it would
+	 * 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;
 
-	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;
 }
-- 
2.28.0

[PATCH 2/2] x86: Make source of unrecognised MSR writes unambiguous

[Chris Down]

In many cases the comm enough 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.

Signed-off-by: Chris Down <chris@chrisdown.name>
Cc: Borislav Petkov <bp@suse.de>
Cc: Jakub Kicinski <kuba@kernel.org>
---
 arch/x86/kernel/msr.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index 3babb0e58b0e..76b6b0011d62 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -99,8 +99,9 @@ static int filter_write(u32 reg)
 	if (!__ratelimit(&fw_rs) || 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 pid %s:%d\n"
+	       "Please report to x86@kernel.org\n",
+	       reg, current->comm, current->pid);
 
 	return 0;
 }
-- 
2.28.0

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@chrisdown.name>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Jakub Kicinski <kuba@kernel.org>
> ---
>  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@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;
>  }
> -- 
> 2.28.0
> 

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] x86: Make source of unrecognised MSR writes unambiguous

[Borislav Petkov]

On Mon, Aug 17, 2020 at 04:24:36PM +0100, Chris Down wrote:
> In many cases the comm enough 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.
> 
> Signed-off-by: Chris Down <chris@chrisdown.name>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Jakub Kicinski <kuba@kernel.org>
> ---
>  arch/x86/kernel/msr.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
> index 3babb0e58b0e..76b6b0011d62 100644
> --- a/arch/x86/kernel/msr.c
> +++ b/arch/x86/kernel/msr.c
> @@ -99,8 +99,9 @@ static int filter_write(u32 reg)
>  	if (!__ratelimit(&fw_rs) || 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 pid %s:%d\n"

Perhaps make that "... by %s (pid: %d)\n".

-- 
Regards/Gruss,
    Boris.

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