Re: [Xen-devel] [PATCH 1/3] x86: allow 64?bit PV guest kernels to suppress user mode exposure of M2P

[Tim Deegan <tim@xxxxxxx>]

Hi,

At 15:54 +0000 on 17 Mar (1426607644), Jan Beulich wrote:
> Xen L4 entries being uniformly installed into any L4 table and 64-bit
> PV kernels running in ring 3 means that user mode was able to see the
> read-only M2P presented by Xen to the guests. While apparently not
> really representing an exploitable information leak, this still very
> certainly was never meant to be that way.
> 
> Building on the fact that these guests already have separate kernel and
> user mode page tables we can allow guest kernels to tell Xen that they
> don't want user mode to see this table. We can't, however, do this by
> default: There is no ABI requirement that kernel and user mode page
> tables be separate. Therefore introduce a new VM-assist flag allowing
> the guest to control respective hypervisor behavior:
> - when not set, L4 tables get created with the respective slot blank,
>   and whenever the L4 table gets used as a kernel one the missing
>   mapping gets inserted,
> - when set, L4 tables get created with the respective slot initialized
>   as before, and whenever the L4 table gets used as a user one the
>   mapping gets zapped.

Right, I think I understand your reasoning here.  Can you put all this
into a header or API doc somewhere?  The comment you add to xen.h is
not nearly enough for an OS dev to figure this out.

Two questions (to which I suspect your answer is yes, but just for clarity):

- Is the ABI change that an always-user-mode context never gets access
  to the RO M2P acceptable, then?  I can't think of a reason why a
  guest would have been relying on the old behaviour, but it is still
  a change.

- Are you happy that this is important enough to add extra code to the
  context switch paths?

> Since the new flag gets assigned a value discontiguous to the existing
> ones (in order to preserve the low bits, as only those are currently
> accessible to 32-bit guests), this requires a little bit of rework of
> the VM assist code in general: An architecture specific
> VM_ASSIST_VALID definition gets introduced (with an optional compat
> mode counterpart), and compilation of the respective code becomes
> conditional upon this being defined (ARM doesn't wire these up and
> hence doesn't need that code).

That all seems fine, but please do it in a separate patch.

I've only one comment on the actual code:

> +            if ( !compat && !VM_ASSIST(d, VMASST_TYPE_m2p_strict) &&
> +                 !paging_mode_refcounts(d) )
> +            {
> +                l4_pgentry_t *l4tab = __map_domain_page(cr3_page);
> +
> +                l4tab[l4_table_offset(RO_MPT_VIRT_START)] =
> +                    idle_pg_table[l4_table_offset(RO_MPT_VIRT_START)];
> +                unmap_domain_page(l4tab);

This map/copy/unmap pattern repeats a lot in this patch -- I think a
helper function would be nice.  Potentially that helper could also
include some of the VMASST/paging_mode_refcounts tests (e.g.
update_l4_ro_mpt_mapping(d, l4mfn, is_compat, is_user_mode)).

Cheers,

Tim.

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