[RFC PATCH] x86/pagewalk: Honor SMAP_CHECK_DISABLED

[RFC PATCH] x86/pagewalk: Honor SMAP_CHECK_DISABLED

[Jason Andryuk]

commit 4c5d78a10dc89427140a50a1df5a0b8e9f073e82 (x86/pagewalk:
Re-implement the pagetable walker) removed honoring the
smap_check_policy of the running VCPU.  guest_walk_tables is used by
copy_{to,from}_guest for HVMs, so it is called when the hypervisor is
copying data and SMAP is inappropriate to enforce.

The out-of-tree v4v hypercall copies a domain's source buffer into a
different domain's destination ring.  For an HVM, the kernel makes the
hypercall from ring 0, so the userspace buffer access looks like a SMAP
violation.  In Xen 4.6, v4v could set SMAP_CHECK_DISABLED to avoid this
SMAP failure, but that no longer works since the re-write.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
commit 31ae587e6f0181bf1f7d196fe1b49357c8922e60 (x86/hvm: always do SMAP
check when updating runstate_guest(v)) added smap_check_policy
originally.  It contained SMAP_CHECK_ENABLED, SMAP_CHECK_DISABLED, and
SMAP_CHECK_HONOR_CPL_AC.  SMAP_CHECK_HONOR_CPL_AC is the default and
conditionalized the SMAP check on the CPL and EFLAGS.AC.
SMAP_CHECK_ENABLED always turned on the SMAP check.

guest_walk_tables no longer has a CPL check.  This seems to be set by
emulation code through the PFEC_user_mode or PFEC_implicit walk
arguments.  Also with the re-write, the EFLAGS.AC check is always
enforced.  So update_runstate_area and update_secondary_system_time may
no longer need the smap policy change?  SMAP_CHECK_HONOR_CPL_AC could
probably be dropped as well if that is the case.

 xen/arch/x86/mm/guest_walk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index 6055fec1ad..627b7f91e8 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -416,6 +416,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
                 goto out;
 
             if ( !(walk & PFEC_insn_fetch) && guest_smap_enabled(v) &&
+                 v->arch.smap_check_policy != SMAP_CHECK_DISABLED &&
                  ((walk & PFEC_implicit) ||
                   !(guest_cpu_user_regs()->eflags & X86_EFLAGS_AC)) )
                 /* User data access and smap? Fail. */
-- 
2.14.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH] x86/pagewalk: Honor SMAP_CHECK_DISABLED

[Andrew Cooper]

On 07/05/2018 20:57, Jason Andryuk wrote:
> commit 4c5d78a10dc89427140a50a1df5a0b8e9f073e82 (x86/pagewalk:
> Re-implement the pagetable walker) removed honoring the
> smap_check_policy of the running VCPU.  guest_walk_tables is used by
> copy_{to,from}_guest for HVMs, so it is called when the hypervisor is
> copying data and SMAP is inappropriate to enforce.
>
> The out-of-tree v4v hypercall copies a domain's source buffer into a
> different domain's destination ring.  For an HVM, the kernel makes the
> hypercall from ring 0, so the userspace buffer access looks like a SMAP
> violation.  In Xen 4.6, v4v could set SMAP_CHECK_DISABLED to avoid this
> SMAP failure, but that no longer works since the re-write.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

I'm sorry, but no.  It is never appropriate to ignore the guest paging
settings.  The correct fix here is in the kernel, to surround the v4v
hypercall handler with stac/clac to whitelist userspace accesses.  See
the implementation of the privcmd hypercall which already does this.

If I could go back in time and nack the introduction of
smap_check_policy, I would.  As it stands, I'm (slowly) removing its
use, and will eventually delete it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH] x86/pagewalk: Honor SMAP_CHECK_DISABLED

[Jason Andryuk]

On Mon, May 7, 2018 at 4:05 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/05/2018 20:57, Jason Andryuk wrote:
>> commit 4c5d78a10dc89427140a50a1df5a0b8e9f073e82 (x86/pagewalk:
>> Re-implement the pagetable walker) removed honoring the
>> smap_check_policy of the running VCPU.  guest_walk_tables is used by
>> copy_{to,from}_guest for HVMs, so it is called when the hypervisor is
>> copying data and SMAP is inappropriate to enforce.
>>
>> The out-of-tree v4v hypercall copies a domain's source buffer into a
>> different domain's destination ring.  For an HVM, the kernel makes the
>> hypercall from ring 0, so the userspace buffer access looks like a SMAP
>> violation.  In Xen 4.6, v4v could set SMAP_CHECK_DISABLED to avoid this
>> SMAP failure, but that no longer works since the re-write.
>>
>> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>
> I'm sorry, but no.  It is never appropriate to ignore the guest paging
> settings.  The correct fix here is in the kernel, to surround the v4v
> hypercall handler with stac/clac to whitelist userspace accesses.  See
> the implementation of the privcmd hypercall which already does this.

Oh, I didn't realize stac/clac are already used with a hypercall.
Thanks for the pointer.

> If I could go back in time and nack the introduction of
> smap_check_policy, I would.  As it stands, I'm (slowly) removing its
> use, and will eventually delete it.

I think you are close.  It seems to me smap_check_policy is set but not used.

Regards,
Jason

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH] x86/pagewalk: Honor SMAP_CHECK_DISABLED

[Andrew Cooper]

On 08/05/18 12:38, Jason Andryuk wrote:
> On Mon, May 7, 2018 at 4:05 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 07/05/2018 20:57, Jason Andryuk wrote:
>>> commit 4c5d78a10dc89427140a50a1df5a0b8e9f073e82 (x86/pagewalk:
>>> Re-implement the pagetable walker) removed honoring the
>>> smap_check_policy of the running VCPU.  guest_walk_tables is used by
>>> copy_{to,from}_guest for HVMs, so it is called when the hypervisor is
>>> copying data and SMAP is inappropriate to enforce.
>>>
>>> The out-of-tree v4v hypercall copies a domain's source buffer into a
>>> different domain's destination ring.  For an HVM, the kernel makes the
>>> hypercall from ring 0, so the userspace buffer access looks like a SMAP
>>> violation.  In Xen 4.6, v4v could set SMAP_CHECK_DISABLED to avoid this
>>> SMAP failure, but that no longer works since the re-write.
>>>
>>> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>> I'm sorry, but no.  It is never appropriate to ignore the guest paging
>> settings.  The correct fix here is in the kernel, to surround the v4v
>> hypercall handler with stac/clac to whitelist userspace accesses.  See
>> the implementation of the privcmd hypercall which already does this.
> Oh, I didn't realize stac/clac are already used with a hypercall.
> Thanks for the pointer.
>
>> If I could go back in time and nack the introduction of
>> smap_check_policy, I would.  As it stands, I'm (slowly) removing its
>> use, and will eventually delete it.
> I think you are close.  It seems to me smap_check_policy is set but not used.

So it is!  Patch incomming.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel