Re: [PATCH 3/3] x86emul: drop "seg" parameter from insn_fetch() hook

[Andrew Cooper <amc96@xxxxxxxx>]

On 03/12/2021 11:23, Jan Beulich wrote:
> This is specified (and asserted for in a number of places) to always be
> CS. Passing this as an argument in various places is therefore
> pointless. The price to pay is two simple new functions,

This is actually a very interesting case study.

Both are indirect targets, so need cf_check (or rather, will do
imminently.  I'll fold a suitable fix when I rebase the CET series).

On the face of it, there's now a pile of parameter shuffling just to get
a 0 in %rdi, which isn't ideal.

However, for fine grained CFI schemes using a type hash, it actually
prevents mixing and matching of read/fetch hooks, so ends up as a
hardening improvement too.

>  with the
> benefit of the PTWR case now gaining a more appropriate error code.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> ---
> In principle in the PTWR case I think we ought to set PFEC_insn_fetch in
> the error code only when NX is seen as available by the guest. Otoh I'd
> kind of expect x86_emul_pagefault() to abstract away this detail.
> Thoughts?

I have mixed feelings.  x86_emul_pagefault() is the wrong place to put
such logic because it, like its neighbours, is just a thin wrapper for
filling the pending event information.

Architecturally, PFEC_insn_fetch is visible for NX || SMEP, and we do
have logic to make this happen correctly for HVM guests (confirmed by my
XTF test, which I *still* need to get around to adding to CI).  I think
it's all contained in the main pagewalk, but I can't remember offhand.

However, PV guests explicitly share their paging settings with Xen, and
we don't hide EFER.NX based on CPUID, although we do appear to hide
CR4.SMEP unilaterally (hardly surprising).

Given the ubiquity of NX these days, and the fact that PV guests are
known-fuzzy in the pagetable department, I'm not sure it's worth the
overhead of trying to hide.

~Andrew