linux-kernel - Re: [PATCH 3/5] mmu: don't set the present bit unconditionally

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: <60e083e8-596a-5641-fcb9-ede8bce32b58@redhat.com>
Date:	Tue, 28 Jun 2016 10:57:45 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Bandan Das <bsd@...hat.com>, kvm@...r.kernel.org
Cc:	guangrong.xiao@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] mmu: don't set the present bit unconditionally



On 28/06/2016 06:32, Bandan Das wrote:
> +	bool execonly = !(context->guest_rsvd_check.bad_mt_xwr &
> +			  (1ull << VMX_EPT_EXECUTABLE_MASK));
>  
>  	if (set_mmio_spte(vcpu, sptep, gfn, pfn, pte_access))
>  		return 0;
>  
> -	spte = PT_PRESENT_MASK;
> +	if (!execonly)
> +		spte |= PT_PRESENT_MASK;

This needs a comment:

	/*
	 * There are two cases in which execonly is false: 1) for
	 * non-EPT page tables, in which case we need to set the
	 * P bit; 2) for EPT page tables where an X-- page table
	 * entry is invalid, in which case we need to force the R
	 * bit of the page table entry to 1.
	 */
	BUILD_BUG_ON(PT_PRESENT_MASK != VMX_EPT_READABLE_MASK);
	if (!execonly)
		spte |= PT_PRESENT_MASK;
	

>  	if (!speculative)
>  		spte |= shadow_accessed_mask;
> 
>  	if (enable_ept) {
> -		kvm_mmu_set_mask_ptes(0ull,
> +		kvm_mmu_set_mask_ptes(PT_PRESENT_MASK,

This should be VMX_EPT_READABLE_MASK.

> @@ -2531,6 +2535,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
>  	else
>  		spte |= shadow_nx_mask;
>  
> +	/* In the EPT case, shadow_user_mask is PT_PRESENT_MASK */

I don't think this comment is necessary, but it's better to add one in
FNAME(gpte_access).

	/*
	 * In the EPT case, a page table can be executable but not
	 * readable (on some processors).  Therefore, set_spte does not
	 * automatically set bit 0 if execute-only is supported.
	 * Instead, since EPT page tables do not have a U bit, we
	 * repurpose ACC_USER_MASK to signify readability.  Likewise,
	 * when EPT is in use shadow_user_mask is set to
	 * VMX_EPT_READABLE_MASK.
	 */
	

Thanks,

Paolo

>  	if (pte_access & ACC_USER_MASK)
>  		spte |= shadow_user_mask;


Paolo

>  			(enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
>  			(enable_ept_ad_bits) ? VMX_EPT_DIRTY_BIT : 0ull,
>  			0ull, VMX_EPT_EXECUTABLE_MASK);