Re: [Xen-devel] [PATCH v3 8/9] x86/hvm: Add SMAP support to HVM guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.14 at 05:17, <feng.wu@xxxxxxxxx> wrote:
> @@ -144,9 +144,10 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>      guest_l4e_t *l4p;
>  #endif
>      uint32_t gflags, mflags, iflags, rc = 0;
> -    int smep;
> +    bool_t smep = 0, smap = 0;
>      bool_t pse1G = 0, pse2M = 0;
>      p2m_query_t qt = P2M_ALLOC | P2M_UNSHARE;
> +    struct segment_register seg;

Please move this into the inner scope below.

> @@ -159,13 +160,31 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain 
> *p2m,
>      mflags = mandatory_flags(v, pfec);
>      iflags = (_PAGE_NX_BIT | _PAGE_INVALID_BITS);
>  
> -    /* SMEP: kernel-mode instruction fetches from user-mode mappings
> -     * should fault.  Unlike NX or invalid bits, we're looking for _all_
> -     * entries in the walk to have _PAGE_USER set, so we need to do the
> -     * whole walk as if it were a user-mode one and then invert the answer. 
> */
> -    smep = (is_hvm_vcpu(v) && hvm_smep_enabled(v) 
> -            && (pfec & PFEC_insn_fetch) && !(pfec & PFEC_user_mode) );
> -    if ( smep )
> +    if ( is_hvm_vcpu(v) && !(pfec & PFEC_user_mode) )
> +    {
> +        struct cpu_user_regs *regs = guest_cpu_user_regs();

Blank line needed between declarations and statements. Also if you
want a local variable for a one time use here (presumably to keep
the line length limited below), please const-qualify it properly.

> +        hvm_get_segment_register(v, x86_seg_ss, &seg);
> +
> +        /* SMEP: kernel-mode instruction fetches from user-mode mappings
> +         * should fault.  Unlike NX or invalid bits, we're looking for _all_
> +         * entries in the walk to have _PAGE_USER set, so we need to do the
> +         * whole walk as if it were a user-mode one and then invert the 
> answer. */
> +        smep =  hvm_smep_enabled(v) && (pfec & PFEC_insn_fetch);
> +
> +        /*
> +         * SMAP: kernel-mode data accesses from user-mode mappings should 
> fault
> +         * A fault is considered as a SMAP violation if the following
> +         * conditions come true:
> +         *   - X86_CR4_SMAP is set in CR4
> +         *   - A user page is accessed
> +         *   - CPL = 3 or X86_EFLAGS_AC is clear
> +         *   - Page fault in kernel mode
> +         */
> +        smap = hvm_smap_enabled(v) &&
> +               !((seg.attr.fields.dpl < 3) && (regs->eflags & 
> X86_EFLAGS_AC));

I think it would be better if you made this match the comment, i.e.

        smap = hvm_smap_enabled(v) &&
               ((seg.attr.fields.dpl == 3) || !(regs->eflags & X86_EFLAGS_AC));

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel