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]

Message-ID: <20180619213255.GH31750@zn.tnic>
Date:   Tue, 19 Jun 2018 23:32:56 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     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 Tue, Jun 19, 2018 at 03:46:40PM -0500, Brijesh Singh wrote:
> 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.

It doesn't matter whether the driver is standalong or not as long as you
specifically have to express explicit dependency on:

depends CRYPTO_DEV_SP_PSP

Now that's the PSP component or whatever but as long as it has a
separate Kconfig symbol, it is the only thing that is important here.

Now, KVM_AMD_SEV needs the functionality behind CRYPTO_DEV_SP_PSP. So
far so good?

> 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
>                                                ....

Let me repeat my point more detailed again because it seems like it is
still not clear:

KVM needs SEV functionality which is behind CRYPTO_DEV_SP_PSP. Which
means, KVM_AMD_SEV should depend *only* on that. This is the symbol
which gives that functionality.

Now, you say "CCP driver manages CCP and PSP devices". So
CRYPTO_DEV_SP_PSP should select/depend on *everything* it needs in order
to function properly in order to provide that functionalty to KVM.

BUT! - and this is the key point - KVM should *not* care what PSP needs
to depend on in order to provide that functionality to KVM. PSP should
provide all that functionality itself and not rely on KVM_AMD_SEV to
select it for the PSP functionality.

IOW, you should have this:

config KVM_AMD_SEV
	bool "AMD Secure Encrypted Virtualization (SEV) support"
	depends KVM_AMD && X86_64 && CRYPTO_DEV_SP_PSP

No more symbols.

CRYPTO_DEV_SP_PSP should then do the proper selection so that
CRYPTO_DEV_CCP_DD has the proper setting wrt whether KVM is a module
or builtin. When you add CRYPTO_DEV_CCP_DD to the dependencies list,
you "fix" the design issue that ccp.ko is the driver which provides PSP
functionality.

I'm not sure if you could do something with a separate symbol as I
suggested earlier:

bool CRYPTO_PSP_FOR_KVM
	depends on CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP

or so and have KVM_AMD_SEV depend on that so that you have one
indirection more which wraps the CCP and PSP dependency.

But that seems unnecessary especially since we already have

config CRYPTO_DEV_SP_PSP
        bool "Platform Security Processor (PSP) device"
        default y
        depends on CRYPTO_DEV_CCP_DD && X86_64

so CRYPTO_DEV_SP_PSP already depends on CRYPTO_DEV_CCP_DD!

So why should KVM_AMD_SEV depend on CRYPTO_DEV_CCP_DD too?!? Do you
catch my drift?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--