linux-kernel - Re: [PATCH 04/16] KVM: arm64: Introduce kvm_share

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: <YW1RphOb9D/4/QGp@google.com>
Date:   Mon, 18 Oct 2021 11:51:18 +0100
From:   Quentin Perret <qperret@...gle.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Fuad Tabba <tabba@...gle.com>,
        David Brazdil <dbrazdil@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 04/16] KVM: arm64: Introduce kvm_share_hyp()

On Sunday 17 Oct 2021 at 11:41:21 (+0100), Marc Zyngier wrote:
> Not directly related to this code, but it looks to me that
> kvm_host_owns_hyp_mappings() really ought to check for
> is_kernel_in_hyp_mode() on its own. VHE really deals with its own
> mappings, and create_hyp_mappings() already has a check to do nothing
> on VHE.

Sure, I'll stick a patch at the beginning of the series.

> 
> > +
> > +	return pkvm_share_hyp(kvm_kaddr_to_phys(from), kvm_kaddr_to_phys(to));
> > +}
> > +
> >  /**
> >   * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
> >   * @from:	The virtual kernel start address of the range
> > @@ -316,12 +327,8 @@ int create_hyp_mappings(void *from, void *to, enum kvm_pgtable_prot prot)
> >  	if (is_kernel_in_hyp_mode())
> >  		return 0;
> >  
> > -	if (!kvm_host_owns_hyp_mappings()) {
> > -		if (WARN_ON(prot != PAGE_HYP))
> > -			return -EPERM;
> > -		return pkvm_share_hyp(kvm_kaddr_to_phys(from),
> > -				      kvm_kaddr_to_phys(to));
> > -	}
> > +	if (WARN_ON(!kvm_host_owns_hyp_mappings()))
> > +		return -EPERM;
> 
> Do we really need this? Can't we just verify that all the code paths
> to create_hyp_mappings() check for kvm_host_owns_hyp_mappings()?
> 
> At the very least, make this a VM_BUG_ON() so that this is limited to
> debug. Yes, I'm quickly developing a WARN_ON()-phobia.

Right, that _is_ purely debug. It's just that folks are used to being
able to just call create_hyp_mappings() for anything, so I wanted to
make sure we have an easy way to catch future changes that would
unknowingly break pKVM, but no objection to make this VM_BUG_ON().

Cheers,
Quentin