linux-kernel - Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

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]

Date:   Tue, 19 Jun 2018 15:46:40 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...e.de>
Cc:     brijesh.singh@....com,
        Richard Weinberger <richard.weinberger@...il.com>,
        Janakarajan Natarajan <Janakarajan.Natarajan@....com>,
        Tom Lendacky <thomas.lendacky@....com>, x86@...nel.org,
        kvm <kvm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krcmar <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, felix@...dspaten.org
Subject: Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies



On 06/19/2018 02:43 PM, Borislav Petkov wrote:
> On Tue, Jun 19, 2018 at 02:22:53PM -0500, Brijesh Singh wrote:
>> It would be nice to have a single depends. But the main issue is, PSP
>> support is provided through the ccp driver (aka CRYPTO_DEV_CCP_DD).
> 
> And this sentence states also your dependency:
> 
> KVM_AMD_SEV <- PSP driver <- CCP driver
> 
> but what you do is jump over one layer
> 
> KVM_AMD_SEV <- PSP driver
> KVM_AMD_SEV <- CCP driver
> PSP driver <- CCP driver
> 
> Do you see the difference and how the former is much easier to express
> as a dependency vs the latter? And thus a lot more robust when it comes to
> random configs.
> 


In case if it was not clear, we don't have a standalone PSP driver.
The PSP support is provided by the CCP driver. If you look at config
changes I proposed then it says if PSP is available then we can support
SEV. But since PSP support is provided by the CCP driver hence we
need to have module dependency with CCP. So, we are using your former 
expression in the dependency but have to extend it a bit more.


>> Hence KVM_AMD_SEV need to have some level of dependency with ccp
>> driver. This is to ensure that the ccp was 'y' when kvm-amd=y for SEV
>> to work.
> 
> If that is so, then your dependencies are wrong. KVM doesn't care about
> CCP driver - it only cares about the PSP driver which gives the SEV API
> it can call. That's it. If you have to make it depend on CCP, then the
> design of what depends on what is wrong.
> 


We had discussion about this during our patch review process but lets
revisit again. CCP driver manages CCP and PSP devices. Ideally the
driver should have been called SP driver but ccp name existed well
before we added high level SP interface. IIRC, during SP patch review it
was recommended not to rename the driver from ccp->sp because it may
break folks who are already using with ccp name.

Here is how the config looks:

                   +------ CRYPTO_DEV_SP_PSP
                   |
CRYPTO_DEV_CCP_DD *
  (ccp.ko)         |                            +-- ccpv3
                   +------ CRYPTO_DEV_SP_CCP  --|
                                                +-- ccpv5
                                                ....




>> I am sorry but I am not able to follow you on how creating a separate CRYPTO
>> .config item will solve this problem. Creating a separate config will be
>> useful if we are okay with calling 'select' from kvm (i.e if kvm-amd is 'y'
>> then all the symbols from crypto will be 'y').
> 
> This is just an example of what you could do.
> 
>> I looked at other drivers where they have similar situation and it seems
>> like solution is same as what I have used above.
> 
> Copying how the others do it is just not good enough - especially since
> people can create broken configs currently.
>