Re: [PATCH] x86/umip: Add a umip= cmdline switch
From: Ricardo Neri
Date: Mon Sep 13 2021 - 17:45:31 EST
On Tue, Sep 07, 2021 at 10:04:54PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@xxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> ---
> .../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