| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 Nov 2017 08:13:21 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...e.de>,
Andy Lutomirski <luto@...nel.org>,
Tony Luck <tony.luck@...el.com>,
Paolo Bonzini <pbonzini@...hat.com>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>, x86@...nel.org,
linux-kernel@...r.kernel.org, ricardo.neri@...el.com
Subject: Re: [PATCH v3] x86/umip: Warn if UMIP-protected instructions are used
* Ricardo Neri <ricardo.neri-calderon@...ux.intel.com> wrote:
> Print a rate-limited warning when a user space program attempts to execute
> any of the instructions that UMIP protects (i.e., SGDT, SIDT, SLDT, STR
> and SMSW).
>
> This is useful because, when CONFIG_X86_INTEL_UMIP is selected and
> supported by the hardware, user space programs that try to execute such
> instructions will receive a SIGSEGV signal that they might not expect.
> In the specific cases for which emulation is provided (instructions SGDT,
> SIDT and SMSW in protected and virtual-8086 modes), no signal is
> generated. However, a warning is helpful to encourage updates in such
> programs to avoid the use of such instructions.
>
> Warnings are printed via a customized printk() function that also provides
> information about the program that attempted to use the affected
> instructions.
>
> Utility macros are defined to wrap umip_printk() for the error and warning
> kernel log levels.
>
> While here, replace an existing call to the generic rate-limited pr_err()
> with the new umip_pr_err().
>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Tony Luck <tony.luck@...el.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Ravi V. Shankar <ravi.v.shankar@...el.com>
> Cc: x86@...nel.org
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> ---
> Changes since V2:
> * Dropped patches 1, 2, and 3 as they were applied on the tip repository.
> * Renamed umip_pr_warn() to umip_printk(). Instead, added utility macros
> for the error and warning kernel log levels.
> * Implemented umip_printk() as a variadic function. This also includes
> using the __printf() macro to type-check arguments of the format
> string and arguments.
> * Capitalized all the instructions' mnemonics in strings used for printing
> warnings.
>
> Changes since V1:
> * Capitalized all the instructions' mnemonics in both code and patch
> descriptions.
> * Corrected documentation of umip_pr_warn() to correctly reflect the function
> name.
> * Updated description of patch #4 to describe the update to the existing
> rate-limited pr_err().
> ---
> arch/x86/kernel/umip.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index 1f1f2d5..dabbac3 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -82,6 +82,57 @@
> #define UMIP_INST_SLDT 3 /* 0F 00 /0 */
> #define UMIP_INST_STR 4 /* 0F 00 /1 */
>
> +const char * const umip_insns[5] = {
> + [UMIP_INST_SGDT] = "SGDT",
> + [UMIP_INST_SIDT] = "SIDT",
> + [UMIP_INST_SMSW] = "SMSW",
> + [UMIP_INST_SLDT] = "SLDT",
> + [UMIP_INST_STR] = "STR",
> +};
> +
> +#define umip_pr_err(regs, fmt, ...) \
> + umip_printk(regs, KERN_ERR, fmt, ##__VA_ARGS__)
> +#define umip_pr_warning(regs, fmt, ...) \
> + umip_printk(regs, KERN_WARNING, fmt, ##__VA_ARGS__)
> +
> +/**
> + * umip_printk() - Print a rate-limited message
> + * @regs: Register set with the context in which the warning is printed
> + * @log_level: Kernel log level to print the message
> + * @fmt: The text string to print
> + *
> + * Print the text contained in @fmt. The print rate is limited to bursts of 5
> + * messages every two minutes. The purpose of this customized version of
> + * printk() is to print messages when user space processes use any of the
> + * UMIP-protected instructions. Thus, the printed text is prepended with the
> + * task name and process ID number of the current task as well as the
> + * instruction and stack pointers in @regs as seen when entering kernel mode.
> + *
> + * Returns:
> + *
> + * None.
> + */
> +static __printf(3, 4)
> +void umip_printk(const struct pt_regs *regs, const char *log_level,
> + const char *fmt, ...)
> +{
> + /* Bursts of 5 messages every two minutes */
> + static DEFINE_RATELIMIT_STATE(ratelimit, 2 * 60 * HZ, 5);
> + struct task_struct *tsk = current;
> + struct va_format vaf;
> + va_list args;
> +
> + if (!__ratelimit(&ratelimit))
> + return;
> +
> + va_start(args, fmt);
> + vaf.fmt = fmt;
> + vaf.va = &args;
> + printk("%s" pr_fmt("%s[%d] ip:%lx sp:%lx: %pV"), log_level, tsk->comm,
> + task_pid_nr(tsk), regs->ip, regs->sp, &vaf);
> + va_end(args);
> +}
> +
> /**
> * identify_insn() - Identify a UMIP-protected instruction
> * @insn: Instruction structure with opcode and ModRM byte.
> @@ -236,10 +287,8 @@ static void force_sig_info_umip_fault(void __user *addr, struct pt_regs *regs)
> if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)))
> return;
>
> - pr_err_ratelimited("%s[%d] umip emulation segfault ip:%lx sp:%lx error:%x in %lx\n",
> - tsk->comm, task_pid_nr(tsk), regs->ip,
> - regs->sp, X86_PF_USER | X86_PF_WRITE,
> - regs->ip);
> + umip_pr_err(regs, "segfault in emulation. error%x\n",
> + X86_PF_USER | X86_PF_WRITE);
> }
>
> /**
> @@ -326,10 +375,15 @@ bool fixup_umip_exception(struct pt_regs *regs)
> if (umip_inst < 0)
> return false;
>
> + umip_pr_warning(regs, "%s instruction cannot be used by applications.\n",
> + umip_insns[umip_inst]);
> +
> /* Do not emulate SLDT, STR or user long mode processes. */
> if (umip_inst == UMIP_INST_STR || umip_inst == UMIP_INST_SLDT || user_64bit_mode(regs))
> return false;
>
> + umip_pr_warning(regs, "For now, expensive software emulation returns the result.\n");
Ok, this looks much nicer all around. I'll test it and push it out if all is good.
Thanks,
Ingo
Powered by blists - more mailing lists