From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4D8AC25B0E for ; Thu, 18 Aug 2022 21:27:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347145AbiHRV1T (ORCPT ); Thu, 18 Aug 2022 17:27:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40166 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347131AbiHRV07 (ORCPT ); Thu, 18 Aug 2022 17:26:59 -0400 Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5E2C8DA3D9 for ; Thu, 18 Aug 2022 14:19:15 -0700 (PDT) Received: by mail-pj1-x102e.google.com with SMTP id r15-20020a17090a1bcf00b001fabf42a11cso3044163pjr.3 for ; Thu, 18 Aug 2022 14:19:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=tTJTjqw4MggpWtxGKCzfbFeqFxFbwJEwUmHLuAFNlC4=; b=KLY0b+bh0hfzUpqKcJhe/wX0zYM3DwWqLn+1EFPX/z/mvu9FX8S6481IG9EwIwKlr0 sksbXHJQ4Kh7CSdeJ54/AGd442l6xfwUSwbgjuIY7cPkTmb/P8QcpBZsFw1/AEgW7BxO Z7vFc1cM6wHyTLrXmXs4BpPTKJqKJm4CDzUsmuDRJO5YqQ2oTdhgmDQfcISIZdvLXi+a iIdIuaNiTdK6EZBOyO8Sqr7BsD7W8hLLdMZHw8f/Q35ywyTfALa1cNGq5gHNhppDaPfZ O+kBBnOCDICeLF6quWO+kUt7vyDNo8jD0dQqib2ZIPNPKJ57Uq0YHy8r7Fi2W5SNMe/A HD3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=tTJTjqw4MggpWtxGKCzfbFeqFxFbwJEwUmHLuAFNlC4=; b=TZVDtsVQ0jhqTnLy2dAOfVWuMYnmXtqfmK4UtYB7QoELyUpQkbgDskT84YRKdMd29L UwEcPCHba7n5dvH74bIFQpjHDQZyqRFd87ul5QHtO7mbSjeAnSrKgWPPVJQPnnIKC5Uy JzqGybtsRlCVb4NA4Rgf467gi4caw1RaQsV9ajRz3xHyKbyMGRpV3hdN5Ol4dKe9J6j1 eMXwYN4In3FgkzGSlWbSVDj8OsW0SxLKjtnc6JRBEC1HwTcx2/CuJ/F6XhpLgjjNzsBN ttORvfH9Ake0ukqHEuaVSNHHo1Zf5IhpFrA14eKSuU9StX8v4yuGnRlkPBEjbMqAfSjk wBrg== X-Gm-Message-State: ACgBeo0WAHNik1P7qyp+66iUW8fc5OS+pnIsr5sphDU1TEvWz9piSGm5 OpJC0xb3zoMJkEAZwUzriXSDgA== X-Google-Smtp-Source: AA6agR6y0AhMETJyQ+vXl0SNQkEBxWGMTcxU9uSEZQLV6JUuiIN57zfeFgll2QLSPF2S39iiIToTNg== X-Received: by 2002:a17:90b:4c4b:b0:1f4:d176:aa5a with SMTP id np11-20020a17090b4c4b00b001f4d176aa5amr4899401pjb.233.1660857553755; Thu, 18 Aug 2022 14:19:13 -0700 (PDT) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id h8-20020a170902f54800b0016ee328fd61sm1819751plf.198.2022.08.18.14.19.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Aug 2022 14:19:13 -0700 (PDT) Date: Thu, 18 Aug 2022 21:19:09 +0000 From: Sean Christopherson To: Kyle Huey Cc: Thomas Gleixner , Dave Hansen , Borislav Petkov , Ingo Molnar , x86@kernel.org, "H. Peter Anvin" , Paolo Bonzini , Andy Lutomirski , Peter Zijlstra , linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, Robert O'Callahan , David Manouchehri , Borislav Petkov , kvm@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH v5 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace. Message-ID: References: <20220808141538.102394-1-khuey@kylehuey.com> <87ilmpzunz.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 18, 2022, Kyle Huey wrote: > On Thu, Aug 18, 2022 at 3:57 AM Thomas Gleixner wrote: > > On Mon, Aug 08 2022 at 07:15, Kyle Huey wrote: > > > When management of the PKRU register was moved away from XSTATE, emulation > > > of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not > > > for APIs that write XSTATE. This can be seen by running gdb and executing > > > `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected kernels (5.14+) the > > > write to the PKRU register (which gdb performs through ptrace) is ignored. > > > > > > There are three relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE, > > > sigreturn, and KVM_SET_XSAVE. KVM_SET_XSAVE has its own special handling to > > > make PKRU writes take effect (in fpu_copy_uabi_to_guest_fpstate). Push that > > > down into copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE > > > and sigreturn pass in pointers to the appropriate PKRU value. > > > > > > This also adds code to initialize the PKRU value to the hardware init value > > > (namely 0) if the PKRU bit is not set in the XSTATE header to match XRSTOR. > > > This is a change to the current KVM_SET_XSAVE behavior. > > > > You are stating a fact here, but provide 0 justification why this is > > correct. > > Well, the justification is that this *is* the behavior we want for > ptrace/sigreturn, and it's very likely the existing KVM_SET_XSAVE > behavior in this edge case is an oversight rather than intentional, > and in the absence of confirmation that KVM wants the existing > behavior (the KVM mailing list and maintainer are CCd) one correct > code path is better than one correct code path and one buggy code > path. Sorry, I missed the KVM-relevant flags. 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);