[PATCH] x86/umip: Add a umip= cmdline switch

[PATCH] x86/umip: Add a umip= cmdline switch

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

And add the first control option

  umip=warnings_off

which disables warnings resulting from emulating UMIP-enabled insns.

The actual use case is for users playing games in wine, games like
Overwatch, for example, which go nuts with SIDT:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

filling up the kernel log unnecessarily with the same info over and over
again which doesn't mean a whit to the users - they just wanna play.

So add a boot-time control switch for those warning messages.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 .../admin-guide/kernel-parameters.txt         |  5 +++
 arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 828d11441ebf..815d022c3e87 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5887,6 +5887,11 @@
 	unknown_nmi_panic
 			[X86] Cause panic on unknown NMI.
 
+	umip=warnings_off
+			[X86]
+			* warnings_off - do not issue warnings when emulating
+			  UMIP-enabled instructions.
+
 	usbcore.authorized_default=
 			[USB] Default USB device authorization:
 			(default -1 = authorized except for wireless USB,
diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e7523d..1d37dc626011 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
 	[UMIP_INST_STR] = "STR",
 };
 
-#define umip_pr_err(regs, fmt, ...) \
+static struct umip_config {
+	__u64 warnings_off	: 1,
+	      __reserved	: 63;
+} umip_cfg;
+
+#define umip_pr_err(regs, fmt, ...)				\
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+
+#define umip_pr_warn(regs, fmt, ...)					\
+({									\
+	if (!umip_cfg.warnings_off)					\
+		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\
+})
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -407,5 +416,23 @@ bool fixup_umip_exception(struct pt_regs *regs)
 
 	/* increase IP to let the program keep going */
 	regs->ip += insn.length;
+
 	return true;
 }
+
+static int __init parse_umip_param(char *str)
+{
+	if (!str)
+		return 0;
+
+	if (*str == '=')
+		str++;
+
+	if (!strcmp(str, "warnings_off"))
+		umip_cfg.warnings_off = 1;
+	else
+		return 0;
+
+	return 1;
+}
+__setup("umip", parse_umip_param);
-- 
2.29.2

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Ricardo Neri]

On Tue, Sep 07, 2021 at 10:04:54PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> And add the first control option
> 
>   umip=warnings_off
> 
> which disables warnings resulting from emulating UMIP-enabled insns.
> 
> The actual use case is for users playing games in wine, games like
> Overwatch, for example, which go nuts with SIDT:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> filling up the kernel log unnecessarily with the same info over and over
> again which doesn't mean a whit to the users - they just wanna play.
> 
> So add a boot-time control switch for those warning messages.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 +++
>  arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 828d11441ebf..815d022c3e87 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5887,6 +5887,11 @@
>  	unknown_nmi_panic
>  			[X86] Cause panic on unknown NMI.
>  
> +	umip=warnings_off
> +			[X86]
> +			* warnings_off - do not issue warnings when emulating
> +			  UMIP-enabled instructions.
> +
>  	usbcore.authorized_default=
>  			[USB] Default USB device authorization:
>  			(default -1 = authorized except for wireless USB,
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..1d37dc626011 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
>  	[UMIP_INST_STR] = "STR",
>  };
>  
> -#define umip_pr_err(regs, fmt, ...) \
> +static struct umip_config {
> +	__u64 warnings_off	: 1,
> +	      __reserved	: 63;
> +} umip_cfg;
> +
> +#define umip_pr_err(regs, fmt, ...)				\
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +
> +#define umip_pr_warn(regs, fmt, ...)					\
> +({									\
> +	if (!umip_cfg.warnings_off)					\
> +		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\

If it is printing the same information again and again, wouldn't it be
simpler to have a umip_pr_warn_once()?

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Borislav Petkov]

On Fri, Sep 10, 2021 at 06:14:59PM -0700, Ricardo Neri wrote:
> If it is printing the same information again and again, wouldn't it be
> simpler to have a umip_pr_warn_once()?

If you do a once thing, you're blocking any other programs from warning,
output you probably wanna see.

With the command line switch you do the same but you're at least pushing
the user to become active and do it first. I.e., with enabling that
option, the user basically says that she/he is not interested in any of
that output and that is ok.

The optimal thing would be to ratelimit it per process but that would be
an overkill and not really needed.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Ricardo Neri]

On Sat, Sep 11, 2021 at 11:20:59AM +0200, Borislav Petkov wrote:
> On Fri, Sep 10, 2021 at 06:14:59PM -0700, Ricardo Neri wrote:
> > If it is printing the same information again and again, wouldn't it be
> > simpler to have a umip_pr_warn_once()?
> 
> If you do a once thing, you're blocking any other programs from warning,
> output you probably wanna see.

That is right. Although, I am not sure programs you can have in
the same machine that also want to use UMIP-protected instructions.
> 
> With the command line switch you do the same but you're at least pushing
> the user to become active and do it first. I.e., with enabling that
> option, the user basically says that she/he is not interested in any of
> that output and that is ok.
> 
> The optimal thing would be to ratelimit it per process but that would be
> an overkill and not really needed.

Indeed.

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Borislav Petkov]

On Mon, Sep 13, 2021 at 02:38:36PM -0700, Ricardo Neri wrote:
> That is right. Although, I am not sure programs you can have in
> the same machine that also want to use UMIP-protected instructions.

Sure, another game. :-P

But srsly, looking at those two:

        umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
                        umip_insns[umip_inst]);

        umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");

Why are they there at all?

I mean, I can hardly imagine userspace doing anything about them.

They're all likely old, arcane applications or games run in wine which
people have no access to the source code anyway so come to think of it,
the once thing is starting to make more sense to me now.

Sure, that:

        umip_pr_err(regs, "segfault in emulation. error%x\n",
                    X86_PF_USER | X86_PF_WRITE);

should be issued unconditionally but I'm wondering if those warning
messages are needed at all. And if not, I should probably simply rip
them all out.

Or at least silence them by default and flip the cmdline switch logic to
enable them for users who are interested in those things but they should
be silent by default.

I.e., you'd need to supply

	umip=warnings_on

on the cmdline to actually see them.

Hmmm?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Ricardo Neri]

On Tue, Sep 14, 2021 at 06:51:22PM +0200, Borislav Petkov wrote:
> On Mon, Sep 13, 2021 at 02:38:36PM -0700, Ricardo Neri wrote:
> > That is right. Although, I am not sure programs you can have in
> > the same machine that also want to use UMIP-protected instructions.
> 
> Sure, another game. :-P
> 
> But srsly, looking at those two:
> 
>         umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
>                         umip_insns[umip_inst]);
> 
>         umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
> 
> Why are they there at all?
> 
> I mean, I can hardly imagine userspace doing anything about them.

The goal at the time was encourage users to report bugs on the
applications and eventually have them fixed. It also meant to warn users
about degraded performance due to emulation. To my knowledge, no one has
reported the latter thus far.

> 
> They're all likely old, arcane applications or games run in wine which
> people have no access to the source code anyway so come to think of it,
> the once thing is starting to make more sense to me now.

Indeed, no one has reported "modern" application using these
instructions.

> 
> Sure, that:
> 
>         umip_pr_err(regs, "segfault in emulation. error%x\n",
>                     X86_PF_USER | X86_PF_WRITE);
> 
> should be issued unconditionally but I'm wondering if those warning
> messages are needed at all. And if not, I should probably simply rip
> them all out.
> 
> Or at least silence them by default and flip the cmdline switch logic to
> enable them for users who are interested in those things but they should
> be silent by defauilt.

Since after almost 4 years, performance degradation does not seem to be a
concern, I think it is sensible to remove the warnings.

> 
> I.e., you'd need to supply
> 
> 	umip=warnings_on
> 
> on the cmdline to actually see them.

They could also be salvaged by converting them to umiip_pr_debug(), just
to err on the cautious side without having to add a new command line
argument.

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Marcus Rückert]

On Wed, 15 Sep 2021 04:34:10 -0700
Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> > They're all likely old, arcane applications or games run in wine
> > which people have no access to the source code anyway so come to
> > think of it, the once thing is starting to make more sense to me
> > now.
> 
> Indeed, no one has reported "modern" application using these
> instructions.

I am not sure if Blizzard Entertainment would tell us why they use this
CPU instruction in Overwatch. And that game is "only" 5 years old.

   darix

-- 
Always remember:
  Never accept the world as it appears to be.
    Dare to see it for what it could be.
      The world can always use more heroes.

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Ricardo Neri]

On Wed, Sep 15, 2021 at 02:21:23PM +0200, Marcus Rückert wrote:
> On Wed, 15 Sep 2021 04:34:10 -0700
> Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote:
> > > They're all likely old, arcane applications or games run in wine
> > > which people have no access to the source code anyway so come to
> > > think of it, the once thing is starting to make more sense to me
> > > now.
> > 
> > Indeed, no one has reported "modern" application using these
> > instructions.
> 
> I am not sure if Blizzard Entertainment would tell us why they use this
> CPU instruction in Overwatch. And that game is "only" 5 years old.

Ah! 5 years old does not seem too old to me. Then it is not only old
applications. Then the warning did catch an app that could in theory be
fixed (if Overwatch is still maintained).

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Borislav Petkov]

On Wed, Sep 15, 2021 at 06:00:07AM -0700, Ricardo Neri wrote:
> Ah! 5 years old does not seem too old to me. Then it is not only old
> applications. Then the warning did catch an app that could in theory be
> fixed (if Overwatch is still maintained).

I wouldn't hold my breath though.

-- 
Regards/Gruss,
    Boris.

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

[PATCH] x86/umip: Downgrade warning messages to debug loglevel

[Borislav Petkov]

On Wed, Sep 15, 2021 at 04:34:10AM -0700, Ricardo Neri wrote:
> The goal at the time was encourage users to report bugs on the
> applications and eventually have them fixed. It also meant to warn users
> about degraded performance due to emulation. To my knowledge, no one has
> reported the latter thus far.

Probably because people do not even get to need UMIP a whole lot,
apparently.

> Since after almost 4 years, performance degradation does not seem to be a
> concern, I think it is sensible to remove the warnings.

Yap.

> They could also be salvaged by converting them to umiip_pr_debug(), just
> to err on the cautious side without having to add a new command line
> argument.

Yap, that's a good idea too:

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 15 Sep 2021 16:39:18 +0200
Subject: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e7523d..5a4b21389b1d 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))
-- 
2.29.2


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/umip: Downgrade warning messages to debug loglevel

[Ricardo Neri]

On Wed, Sep 15, 2021 at 04:46:20PM +0200, Borislav Petkov wrote:
> On Wed, Sep 15, 2021 at 04:34:10AM -0700, Ricardo Neri wrote:
> > The goal at the time was encourage users to report bugs on the
> > applications and eventually have them fixed. It also meant to warn users
> > about degraded performance due to emulation. To my knowledge, no one has
> > reported the latter thus far.
> 
> Probably because people do not even get to need UMIP a whole lot,
> apparently.
> 
> > Since after almost 4 years, performance degradation does not seem to be a
> > concern, I think it is sensible to remove the warnings.
> 
> Yap.
> 
> > They could also be salvaged by converting them to umiip_pr_debug(), just
> > to err on the cautious side without having to add a new command line
> > argument.
> 
> Yap, that's a good idea too:
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Wed, 15 Sep 2021 16:39:18 +0200
> Subject: [PATCH] x86/umip: Downgrade warning messages to debug loglevel
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> After four years in the wild, those have not fullfilled their
> initial purpose of pushing people to fix their software to not use
> UMIP-emulated instructions, and to warn users about the degraded
> emulation performance.
> 
> Yet, the only thing that "degrades" performance is overflowing dmesg
> with those:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> and users don't really care about that - they just want to play their
> games in wine.
> 
> So convert those to debug loglevel - in case someone is still interested
> in them, someone can boot with "debug" on the kernel cmdline.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
> ---
>  arch/x86/kernel/umip.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..5a4b21389b1d 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
>  
>  #define umip_pr_err(regs, fmt, ...) \
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +#define umip_pr_debug(regs, fmt, ...) \
> +	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
>  
>  /**
>   * umip_printk() - Print a rate-limited message
> @@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
>  	if (umip_inst < 0)
>  		return false;
>  
> -	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
> +	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
>  			umip_insns[umip_inst]);
>  
> -	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
> +	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
>  
>  	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
>  			      user_64bit_mode(regs)))

FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Downgrade warning messages to debug loglevel

[Borislav Petkov]

On Wed, Sep 15, 2021 at 05:27:35PM -0700, Ricardo Neri wrote:
> FWIW, Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>

Thx.

So this thing is not completely quiet:

# dmesg -l err,warn,notice -x
...
kern  :notice: [   17.974749] random: dd: uninitialized urandom read (512 bytes read)
kern  :notice: [   18.063596] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   18.065212] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   20.161303] random: crng init done
kern  :warn  : [  144.505690] umip_printk: 403267 callbacks suppressed
^^^^^^^^^^^^^^^^

so at least it says that umip is being busy logging stuff and if one
wants to see them, one can do:

# dmesg -l err,warn,notice,debug -x
kern  :notice: [   17.974749] random: dd: uninitialized urandom read (512 bytes read)
kern  :notice: [   18.063596] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   18.065212] random: dbus-daemon: uninitialized urandom read (12 bytes read)
kern  :notice: [   20.161303] random: crng init done
kern  :debug : [   24.501738] umip: sidt[1882] ip:5570e245a083 sp:7ffdbe080390: SIDT instruction cannot be used by applications.
kern  :debug : [   24.501743] umip: sidt[1882] ip:5570e245a083 sp:7ffdbe080390: For now, expensive software emulation returns the result.
kern  :debug : [   24.502566] umip: sidt[1883] ip:55c63b5ff083 sp:7fffde333160: SIDT instruction cannot be used by applications.
kern  :debug : [   24.502569] umip: sidt[1883] ip:55c63b5ff083 sp:7fffde333160: For now, expensive software emulation returns the result.
kern  :debug : [   24.503259] umip: sidt[1884] ip:55e4bff9e083 sp:7ffecb324530: SIDT instruction cannot be used by applications.
kern  :warn  : [  144.505690] umip_printk: 403267 callbacks suppressed
kern  :debug : [  144.505692] umip: sidt[9170] ip:560d1457f083 sp:7fff1d5d9df0: SIDT instruction cannot be used by applications.
kern  :debug : [  144.506484] umip: sidt[9170] ip:560d1457f083 sp:7fff1d5d9df0: For now, expensive software emulation returns the result.
kern  :debug : [  144.507218] umip: sidt[9171] ip:556f4549c083 sp:7ffc146ab2a0: SIDT instruction cannot be used by applications.
kern  :debug : [  144.507221] umip: sidt[9171] ip:556f4549c083 sp:7ffc146ab2a0: For now, expensive software emulation returns the result.
kern  :debug : [  144.507951] umip: sidt[9172] ip:55662fdd8083 sp:7ffefcdbc0d0: SIDT instruction cannot be used by applications.

ok, lemme queue it.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Ricardo Neri]

On Tue, Sep 07, 2021 at 10:04:54PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> And add the first control option
> 
>   umip=warnings_off
> 
> which disables warnings resulting from emulating UMIP-enabled insns.
> 
> The actual use case is for users playing games in wine, games like
> Overwatch, for example, which go nuts with SIDT:
> 
>   [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
>   [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
>   ...
>   [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
>   [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.
> 
> filling up the kernel log unnecessarily with the same info over and over
> again which doesn't mean a whit to the users - they just wanna play.
> 
> So add a boot-time control switch for those warning messages.
> 
> Reported-by: Marcus Rückert <mrueckert@suse.com>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 +++
>  arch/x86/kernel/umip.c                        | 33 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 828d11441ebf..815d022c3e87 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5887,6 +5887,11 @@
>  	unknown_nmi_panic
>  			[X86] Cause panic on unknown NMI.
>  
> +	umip=warnings_off
> +			[X86]
> +			* warnings_off - do not issue warnings when emulating
> +			  UMIP-enabled instructions.
> +
>  	usbcore.authorized_default=
>  			[USB] Default USB device authorization:
>  			(default -1 = authorized except for wireless USB,
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 576b47e7523d..1d37dc626011 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -90,10 +90,19 @@ static const char * const umip_insns[5] = {
>  	[UMIP_INST_STR] = "STR",
>  };
>  
> -#define umip_pr_err(regs, fmt, ...) \
> +static struct umip_config {
> +	__u64 warnings_off	: 1,
> +	      __reserved	: 63;
> +} umip_cfg;
> +
> +#define umip_pr_err(regs, fmt, ...)				\
>  	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> -#define umip_pr_warn(regs, fmt, ...) \
> -	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
> +
> +#define umip_pr_warn(regs, fmt, ...)					\
> +({									\
> +	if (!umip_cfg.warnings_off)					\
> +		umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__);	\
> +})
>  
>  /**
>   * umip_printk() - Print a rate-limited message
> @@ -407,5 +416,23 @@ bool fixup_umip_exception(struct pt_regs *regs)
>  
>  	/* increase IP to let the program keep going */
>  	regs->ip += insn.length;
> +

Strictly, this hunk does not logically belong to this patch. I guess it
is harmless, though. Admittedly, the new line should have been there
since umip.c was added.

>  	return true;
>  }
> +
> +static int __init parse_umip_param(char *str)
> +{
> +	if (!str)
> +		return 0;
> +
> +	if (*str == '=')
> +		str++;
> +
> +	if (!strcmp(str, "warnings_off"))
> +		umip_cfg.warnings_off = 1;
> +	else
> +		return 0;
> +
> +	return 1;
> +}
> +__setup("umip", parse_umip_param);

Wouldn't it be better to use parse_option_str() with __setup("umip=",
parse_umip_param)? This would avoid the first two checks.

Thanks and BR,
Ricardo

Re: [PATCH] x86/umip: Add a umip= cmdline switch

[Borislav Petkov]

On Mon, Sep 13, 2021 at 02:45:03PM -0700, Ricardo Neri wrote:
> Strictly, this hunk does not logically belong to this patch. I guess it
> is harmless, though. Admittedly, the new line should have been there
> since umip.c was added.

Yeah, I don't like crammed code - that's why I've added it.

> Wouldn't it be better to use parse_option_str() with __setup("umip=",
> parse_umip_param)? This would avoid the first two checks.

Sure.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel

[tip-bot2 for Borislav Petkov]

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     1eefe7a85678a056951cd9efb09820c1b0a1b4da
Gitweb:        https://git.kernel.org/tip/1eefe7a85678a056951cd9efb09820c1b0a1b4da
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Wed, 15 Sep 2021 16:39:18 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 23 Sep 2021 16:30:11 +02:00

x86/umip: Downgrade warning messages to debug loglevel

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e..5a4b213 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))

[tip: x86/cpu] x86/umip: Downgrade warning messages to debug loglevel

[tip-bot2 for Borislav Petkov]

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     f3f07ae425bc09039d9e0c73c86b76f95d9d5cd6
Gitweb:        https://git.kernel.org/tip/f3f07ae425bc09039d9e0c73c86b76f95d9d5cd6
Author:        Borislav Petkov <bp@suse.de>
AuthorDate:    Wed, 15 Sep 2021 16:39:18 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Sat, 25 Sep 2021 13:23:28 +02:00

x86/umip: Downgrade warning messages to debug loglevel

After four years in the wild, those have not fullfilled their
initial purpose of pushing people to fix their software to not use
UMIP-emulated instructions, and to warn users about the degraded
emulation performance.

Yet, the only thing that "degrades" performance is overflowing dmesg
with those:

  [Di Sep  7 00:24:05 2021] umip_printk: 1345 callbacks suppressed
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: SIDT instruction cannot be used by applications.
  [Di Sep  7 00:24:05 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b7c0: For now, expensive software emulation returns the result.
  ...
  [Di Sep  7 00:26:06 2021] umip_printk: 2227 callbacks suppressed
  [Di Sep  7 00:26:06 2021] umip: someapp.exe[29231] ip:14064cdba sp:11b940: SIDT instruction cannot be used by applications.

and users don't really care about that - they just want to play their
games in wine.

So convert those to debug loglevel - in case someone is still interested
in them, someone can boot with "debug" on the kernel cmdline.

Reported-by: Marcus Rückert <mrueckert@suse.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Link: https://lkml.kernel.org/r/20210907200454.30458-1-bp@alien8.de
---
 arch/x86/kernel/umip.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index 576b47e..5a4b213 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -92,8 +92,8 @@ static const char * const umip_insns[5] = {
 
 #define umip_pr_err(regs, fmt, ...) \
 	umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
-#define umip_pr_warn(regs, fmt, ...) \
-	umip_printk(regs, KERN_WARNING, fmt,  ##__VA_ARGS__)
+#define umip_pr_debug(regs, fmt, ...) \
+	umip_printk(regs, KERN_DEBUG, fmt,  ##__VA_ARGS__)
 
 /**
  * umip_printk() - Print a rate-limited message
@@ -361,10 +361,10 @@ bool fixup_umip_exception(struct pt_regs *regs)
 	if (umip_inst < 0)
 		return false;
 
-	umip_pr_warn(regs, "%s instruction cannot be used by applications.\n",
+	umip_pr_debug(regs, "%s instruction cannot be used by applications.\n",
 			umip_insns[umip_inst]);
 
-	umip_pr_warn(regs, "For now, expensive software emulation returns the result.\n");
+	umip_pr_debug(regs, "For now, expensive software emulation returns the result.\n");
 
 	if (emulate_umip_insn(&insn, umip_inst, dummy_data, &dummy_data_size,
 			      user_64bit_mode(regs)))