linux-kernel - Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to protect dynamic user state

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <E5C66E6E-8AB8-4258-814D-C8E94446E2C4@intel.com>
Date:   Sun, 3 Oct 2021 22:38:29 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "bp@...e.de" <bp@...e.de>, "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Macieira, Thiago" <thiago.macieira@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 13/28] x86/fpu/xstate: Use feature disable (XFD) to
 protect dynamic user state

On Oct 1, 2021, at 08:10, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Fri, Oct 01 2021 at 17:02, Thomas Gleixner wrote:
>> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>>> DEFINE_IDTENTRY(exc_device_not_available)
>>> {
>>> 	unsigned long cr0 = read_cr0();
>> 
>>> +	if (handle_xfd_event(&current->thread.fpu, regs))
>>> +		return;
>> 
>> As I said before, this is wrong because at that point interrupts are disabled.
> 
> So you want something like this:
> 
> static bool handle_xfd_event(struct pt_regs *regs)
> {
> 	u64 xfd_err, xfd_event, xfd, mask;
> 	struct fpu *fpu;
> 
> 	if (!cpu_feature_enabled(X86_FEATURE_XFD))
> 		return false;
> 
> 	rdmsrl_safe(MSR_IA32_XFD_ERR, &xfd_err);
> 	if (!xfd_err)
> 		return false;
> 
> 	wrmsrl_safe(MSR_IA32_XFD_ERR, 0);
> 
> 	xfd_event = xfd_err & xfeatures_mask_user_dynamic;
> 
> 	/* Die if a non-handled feature raised the exception */
> 	if (WARN_ON(!xfd_event))
> 		return true;
> 
> 	/* Die if that happens in kernel space */
> 	if (WARN_ON(!user_mode(regs)))
> 		return false;
> 
> 	local_irq_enable();

v1 had some similar ones (not the same though) [1]. FWIW, I think Andy’s point
is worth to be noted here:

    First, you can't just enable IRQs here. If IRQs are off, they're off for a
    reason. Secondly, if they're *on*, you just forgot that fact.

> 	/* Make sure that dynamic buffer expansion is permitted. */
> 	if (dynamic_state_permitted(current, xfd_event) &&
> 	    !realloc_xstate_buffer(current, xfd_event)) {
> 		mask = xfeatures_mask_user_dynamic;
>                fpu = &current->thread.fpu;
> 		xfd_write((fpu->state_mask & mask) ^ mask);
> 	} else {
> 		force_sig_fault(SIGILL, ILL_ILLOPC, error_get_trap_addr(regs));
> 	}
> 
> 	local_irq_disable();
> 	return true;
> }
> 
> Along with a correct implementation of realloc_xstate_buffer().

Thanks,
Chang

[1] https://lore.kernel.org/lkml/CALCETrWjOYd4wM0Mn7fY+t4ztU99GNP77A6skNwjTViJYUEZYQ@mail.gmail.com/