Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/03/2014 18:36, Feng Wu ha scritto:
> +	smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);

You are overwriting this variable below, but that is not okay because
the value of CR4 must be considered separately in each iteration.  This
also hides a uninitialized-variable bug for "smap" correctly in the EPT
case.

To avoid that, rename this variable to cr4_smap; it's probably better 
to rename smep to cr4_smep too.

>  	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
>  		pfec = byte << 1;
>  		map = 0;
>  		wf = pfec & PFERR_WRITE_MASK;
>  		uf = pfec & PFERR_USER_MASK;
>  		ff = pfec & PFERR_FETCH_MASK;
> +		smapf = pfec & PFERR_RSVD_MASK;

The reader will expect PFERR_RSVD_MASK to be zero here.  So please
add a comment: /* PFERR_RSVD_MASK is set in pfec if ... */".

>  		for (bit = 0; bit < 8; ++bit) {
>  			x = bit & ACC_EXEC_MASK;
>  			w = bit & ACC_WRITE_MASK;
> @@ -3627,11 +3629,27 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  				w |= !is_write_protection(vcpu) && !uf;
>  				/* Disallow supervisor fetches of user code if cr4.smep */
>  				x &= !(smep && u && !uf);
> +
> +				/*
> +				 * SMAP:kernel-mode data accesses from user-mode
> +				 * mappings should fault. A fault is considered
> +				 * as a SMAP violation if all of the following
> +				 * conditions are ture:
> +				 *   - X86_CR4_SMAP is set in CR4
> +				 *   - An user page is accessed
> +				 *   - Page fault in kernel mode
> +				 *   - !(CPL<3 && X86_EFLAGS_AC is set)
> +				 *
> +				 *   Here, we cover the first three conditions,
> +				 *   we need to check CPL and X86_EFLAGS_AC in
> +				 *   permission_fault() dynamiccally

"dynamically".  These three lines however are not entirely correct.  We do
cover the last condition here, it is in smapf.  So perhaps something like

 * Here, we cover the first three conditions.
 * The CPL and X86_EFLAGS_AC is in smapf, which
 * permission_fault() computes dynamically.

> +				 */
> +				smap = smap && smapf && u && !uf;

SMAP does not affect instruction fetches.  Do you need "&& !ff" here?  Perhaps
it's clearer to add it even if it is not strictly necessary.

Please write just "smap = cr4_smap && u && !uf && !ff" here, and add back smapf below
in the assignment to "fault".  This makes the code more homogeneous.

>  			} else
>  				/* Not really needed: no U/S accesses on ept  */
>  				u = 1;
> -			fault = (ff && !x) || (uf && !u) || (wf && !w);
> +			fault = (ff && !x) || (uf && !u) || (wf && !w) || smap;

...

> +
> +	/*
> +	 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
> +	 *
> +	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> +	 * (these are implicit supervisor accesses) regardless of the value
> +	 * of EFLAGS.AC.
> +	 *
> +	 * So we need to check CPL and EFLAGS.AC to detect whether there is
> +	 * a SMAP violation.
> +	 */
> +
> +	smapf = ((mmu->permissions[(pfec|PFERR_RSVD_MASK) >> 1] >> pte_access) &
> +		 1) && !((cpl < 3) && ((rflags & X86_EFLAGS_AC) == 1));
> +
> +	return ((mmu->permissions[pfec >> 1] >> pte_access) & 1) || smapf;

You do not need two separate accesses.  Just add PFERR_RSVD_MASK to pfec if
the conditions for SMAP are satisfied.  There are two possibilities:

1) setting PFERR_RSVD_MASK if SMAP is being enforced, that is if CPL = 3
|| AC = 0.  This is what you are doing now.

2) setting PFERR_RSVD_MASK if SMAP is being overridden, that is if CPL < 3
&& AC = 1.  You then have to invert the bit in update_permission_bitmask.

Please consider both choices, and pick the one that gives better code.

Also, this must be written in a branchless way.  Branchless tricks are common
throughout the MMU code because the hit rate of most branches is pretty much
50%-50%.  This is also true in this case, at least if SMAP is in use (if it
is not in use, we'll have AC=0 most of the time).

I don't want to spoil the fun, but I don't want to waste your time either
so I rot13'ed my solution and placed it after the signature. ;)

As to nested virtualization, I reread the code and it should already work,
because it sets PFERR_USER_MASK.  This means uf=1, and a SMAP fault will
never trigger with uf=1.

Thanks for following this!  Please include "v3" in the patch subject on
your next post!

Paolo

------------------------------------- 8< --------------------------------------
Nqq qrsvavgvbaf sbe CSREE_*_OVG (0 sbe cerfrag, 1 sbe jevgr, rgp.) naq
hfr gur sbyybjvat:

        vag vaqrk, fznc;

        /*
         * Vs PCY < 3, FZNC cebgrpgvbaf ner qvfnoyrq vs RSYNTF.NP = 1.
         *
         * Vs PCY = 3, FZNC nccyvrf gb nyy fhcreivfbe-zbqr qngn npprffrf
         * (gurfr ner vzcyvpvg fhcreivfbe npprffrf) ertneqyrff bs gur inyhr
         * bs RSYNTF.NP.
         *
         * Guvf pbzchgrf (pcy < 3) && (esyntf & K86_RSYNTF_NP), yrnivat
         * gur erfhyg va K86_RSYNTF_NP.  Jr gura vafreg vg va cynpr
         * bs gur CSREE_EFIQ_ZNFX ovg; guvf ovg jvyy nyjnlf or mreb va csrp,
         * ohg vg jvyy or bar va vaqrk vs FZNC purpxf ner orvat bireevqqra.
         * Vg vf vzcbegnag gb xrrc guvf oenapuyrff.
         */
        fznc = (pcy - 3) & (esyntf & K86_RSYNTF_NP);
        vaqrk =
           (csrp >> 1) +
           (fznc >> (K86_RSYNTF_NP_OVG - CSREE_EFIQ_OVG + 1));

        erghea (zzh->crezvffvbaf[vaqrk] >> cgr_npprff) & 1;

Gur qverpgvba bs CSREE_EFIQ_ZNFX vf gur bccbfvgr pbzcnerq gb lbhe pbqr.