linux-kernel - Re: [RESEND PATCH v6 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace.

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] [day] [month] [year] [list]

Date:   Fri, 11 Nov 2022 17:00:43 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Kyle Huey <me@...ehuey.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        Robert O'Callahan <robert@...llahan.org>,
        David Manouchehri <david.manouchehri@...eup.net>,
        Borislav Petkov <bp@...e.de>, stable@...r.kernel.org
Subject: Re: [RESEND PATCH v6 1/2] x86/fpu: Allow PKRU to be (once again)
 written by ptrace.

On Thu, Nov 10, 2022, Dave Hansen wrote:
> On 11/10/22 16:03, Kyle Huey wrote:
> > On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@...el.com> wrote:
> BTW, I'd love to know if KVM *REALLY* depends on this.

Unlikely, but nearly impossible to know for sure.  Copy+pasting my response[1] to
an earlier version.

 : Hrm, the current behavior has been KVM ABI for a very long time.
 : 
 : It's definitely odd because all other components will be initialized due to their
 : bits being cleared in the header during kvm_load_guest_fpu(), and it probably
 : wouldn't cause problems in practice as most VMMs likely do "all or nothing" loads.
 : But, in theory, userspace could save/restore a subset of guest XSTATE and rely on
 : the kernel not overwriting guest PKRU when its bit is cleared in the header.
 : 
 : All that said, I don't see any reason to force KVM to change at this time, it's
 : trivial enough to handle KVM's oddities while providing sane behavior for others.
 : Nullify the pointer in the guest path and then update copy_uabi_to_xstate() to
 : play nice with a NULL pointer, e.g. 
 : 
 : 	/*
 : 	 * Nullify @vpkru to preserve its current value if PKRU's bit isn't set
 : 	 * in the header.  KVM's odd ABI is to leave PKRU untouched in this
 : 	 * case (all other components are eventually re-initialized).
 : 	 */
 : 	if (!(kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU))
 : 		vpkru = NULL;
 : 
 : 	return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);

> It'd be nice to kill if not.

I don't disagree, my hesitation is purely that doing so might subtly break
userspace.

That said, I'm 99.9% certain no traditional VMM, e.g. QEMU, is relying on this
behavior, as doing KVM_SET_XSAVE with anything except the guest's xfeatures mask
would corrupt guest XSAVE state for everything except PKRU.  I.e. for all intents
and purposes, a traditional VMM must do KVM_GET_SAVE => KVM_SET_XSAVE without
touching the xfeatures mask.

And for non-traditional usage of KVM, I would be quite surprised if any of those
use cases utilize PKRU in the guest, let alone play games with KVM_{G,S}SET_XSAVE.

So, I'm not completely opposed to "fixing" KVM's ABI, but it should be done as a
separate patch that is tagged "KVM: x86:" and clearly states that it's changing
KVM's ABI in a way that could theoretically break userspace.

> >> Would something like this be more clear?
> >>
> >>         if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
> >>                 struct pkru_state *xpkru;
> >>
> >>                 xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
> >>                 *pkru = xpkru->pkru;
> >>         } else {
> >>                 /*
> >>                  * KVM may pass a NULL 'pkru' to indicate
> >>                  * that it does not need PKRU updated.
> >>                  */
> >>                 if (pkru)
> >>                         *pkru = 0;
> >>         }
> > 
> > Yeah, Sean Christopherson suggested this (with the else and if
> > collapsed into a single level) when I submitted this previously.
> 
> I generally agree with Sean, but he's also been guilty of an atrocity or
> two over the years.  :)

Heh, just one or two?  I'll call that a win.

> While I generally like low levels of indentation I also think my version is
> much more clear in this case.

I've no objection to a standalone if.  My suggestion[2] was in response to code
that zeroed @pkru before the XFEATURE_MASK_PKRU check.

      if (pkru)
              *pkru = 0;

      if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
              struct pkru_state *xpkru;
              xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
              *pkru = xpkru->pkru;
      }

[1] https://lore.kernel.org/all/Yv6szXuKGv75wWmm@google.com
[2] https://lore.kernel.org/all/YxDP6jie4cwzZIHp@google.com