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

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

Date:   Wed, 23 May 2018 16:46:21 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Janakarajan Natarajan <Janakarajan.Natarajan@....com>,
        Brijesh Singh <brijesh.singh@....com>,
        Tom Lendacky <thomas.lendacky@....com>
Cc:     x86@...nel.org, kvm@...r.kernel.org, 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>
Subject: Re: [PATCH] Use 'imply' with SEV Kconfig CRYPTO dependencies

+ Tom and Brijesh.

On Mon, May 21, 2018 at 10:12:53AM -0500, Janakarajan Natarajan wrote:
> Use Kconfig imply 'option' when specifying SEV CRYPTO dependencies.
> 
> Example configuration:
> .
> .
> CONFIG_CRYPTO_DEV_CCP=y
> CONFIG_CRYPTO_DEV_CCP_DD=m
> CONFIG_CRYPTO_DEV_SP_CCP=y
> CONFIG_CRYPTO_DEV_CCP_CRYPTO=m
> CONFIG_CRYPTO_DEV_SP_PSP=y
> .
> .
> CONFIG_KVM_AMD=y
> CONFIG_KVM_AMD_SEV=y
> .
> .
> 
> When the CRYPTO_DEV_SP_DD is m, KVM_AMD set to y produces compile time
> errors.
> 
> Since KVM_AMD_SEV depends on KVM_AMD and CRYPTO_DEV_CCP_DD, the
> issue can be prevented by using 'imply' Kconfig option when specifying
> the CRYPTO dependencies.
> 
> Fixes: 505c9e94d832 ("KVM: x86: prefer "depends on" to "select" for SEV")
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@....com>
> ---
>  arch/x86/kvm/Kconfig | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index 92fd433..d9b16b7 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -85,7 +85,9 @@ config KVM_AMD_SEV
>  	def_bool y
>  	bool "AMD Secure Encrypted Virtualization (SEV) support"
>  	depends on KVM_AMD && X86_64
> -	depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP
> +	imply CRYPTO_DEV_CCP
> +	imply CRYPTO_DEV_CCP_DD
> +	imply CRYPTO_DEV_SP_PSP
>  	---help---
>  	Provides support for launching Encrypted VMs on AMD processors.

Well, this solves the build issue but I just created a config:

$ grep -E "(KVM|PSP)" .config | grep -v '#'
CONFIG_HAVE_KVM=y
CONFIG_HAVE_KVM_IRQCHIP=y
CONFIG_HAVE_KVM_IRQFD=y
CONFIG_HAVE_KVM_IRQ_ROUTING=y
CONFIG_HAVE_KVM_EVENTFD=y
CONFIG_KVM_MMIO=y
CONFIG_KVM_ASYNC_PF=y
CONFIG_HAVE_KVM_MSI=y
CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT=y
CONFIG_KVM_VFIO=y
CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y
CONFIG_KVM_COMPAT=y
CONFIG_HAVE_KVM_IRQ_BYPASS=y
CONFIG_KVM=y
CONFIG_KVM_AMD=m

which builds but the PSP functionality is not there. And I don't think
this is serving the user: she should be able to select what she wants
and get the required functionality added and not have build errors with
any possible configuration.

Now, looking at the security processor Kconfig stuff, it is somewhat
confusing but maybe I didn't look at it long enough. A couple of points:

config CRYPTO_DEV_CCP_DD
        tristate "Secure Processor device driver"

If this is going to be the top-level menu item for the SP, call that

 CRYPTO_DEV_SP

to mean, this is the security processor. CCP_DD is confusing because you
have CRYPTO_DEV_SP_CCP which is the crypto coprocessor support.

And "DD" for device driver is a pure tautology - most of the Kconfig
items are device drivers :)

Then,

 config KVM_AMD_SEV
        def_bool y
        bool "AMD Secure Encrypted Virtualization (SEV) support"
        depends on KVM_AMD && X86_64
        depends on CRYPTO_DEV_CCP && CRYPTO_DEV_CCP_DD && CRYPTO_DEV_SP_PSP

that last line is pulling the required functionality for SEV but - and
I *think* we have talked about this before - having a hierarchical
dependency should make this a lot clearer and fix the build issues along
the way.

Because, IMHO, KVM_AMD_SEV should depend only on CRYPTO_DEV_SP_PSP -
i.e., the PSP device because SEV needs the PSP, right?

Now, the PSP device *itself* should depend on whatever it needs to
function properly, CRYPTO_DEV_CCP_DD I guess.

But SEV should not depend on CRYPTO_DEV_CCP - which is the top-level
Kconfig item - that should be expressed implicitly through the
dependency chain where PSP ends up depending on it.

So that you have one-hop deps:

KVM_SEV -> PSP -> CCP -> ...

IOW, a config item, say PSP - if enabled - should make sure it
selects/depends on everything it needs to function. The upper level
item KVM_SEV - which selects/depends on that config item shouldn't
be responsible for making sure the correct items for PSP's proper
functioning are enabled - that's PSP's item's job.

Makes sense?

Maybe I'm missing something but applying this simple logic would prevent
such Kconfig build issues and make the whole dependency chain almost
trivial.

Thx.

-- 
Regards/Gruss,
    Boris.

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