linux-kernel - Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f7e6b2f444f34064e34d7bd680d2c863b9ce6a41.camel@kernel.org>
Date:   Fri, 03 Sep 2021 18:58:55 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Tony Luck <tony.luck@...el.com>, linux-sgx@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH] x86/sgx: Declare sgx_set_attribute() for !CONFIG_X86_SGX

On Fri, 2021-09-03 at 15:29 +0000, Sean Christopherson wrote:
> On Fri, Sep 03, 2021, Jarkko Sakkinen wrote:
> > Simplify sgx_set_attribute() usage by declaring a fallback
> > implementation for it rather than requiring to have compilation
> > flag checks in the call site. The fallback unconditionally returns
> > -EINVAL.
> > 
> > Refactor the call site in kvm_vm_ioctl_enable_cap() accordingly.
> > The net result is the same: KVM_CAP_SGX_ATTRIBUTE causes -EINVAL
> > when kernel is compiled without CONFIG_X86_SGX_KVM.
> 
> Eh, it doesn't really simplify the usage.  If anything it makes it more convoluted
> because the capability check in kvm_vm_ioctl_check_extension() still needs an
> #ifdef, e.g. readers will wonder why the check is conditional but the usage is not.

It does objectively a bit, since it's one ifdef less.

This is fairly standard practice to do in kernel APIs, used in countless
places, for instance in Tony's patch set to add MCE recovery for SGX. And
it would be nice to share common pattern here how we define API now and
futre.

I also remarked that declaration of "sgx_provisioning_allowed" is not flagged,
which is IMHO even more convolved because without SGX it is spare data.

/Jarkko