The conversion to __get_guest() failed to account for the fact that for
remote vCPU-s dumping gets done through a pointer obtained from
map_domain_page(): __get_guest() arranges for (apparent) accesses to
hypervisor space to cause #GP(0).

Fixes: 6a1d72d3739e ('x86: split __{get,put}_user() into "guest" and "unsafe" variants')
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Using get_unsafe() might be an option as well, instead of the added
extra conditional.

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -275,7 +275,9 @@ static void compat_show_guest_stack(stru
     {
         if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
             break;
-        if ( __get_guest(addr, stack) )
+        if ( stack_page )
+            addr = *stack;
+        else if ( __get_guest(addr, stack) )
         {
             if ( i != 0 )
                 printk("\n    ");
@@ -344,7 +346,9 @@ static void show_guest_stack(struct vcpu
     {
         if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
             break;
-        if ( __get_guest(addr, stack) )
+        if ( stack_page )
+            addr = *stack;
+        else if ( __get_guest(addr, stack) )
         {
             if ( i != 0 )
                 printk("\n    ");

On Wed, Sep 29, 2021 at 11:42:34AM +0200, Jan Beulich wrote:
> The conversion to __get_guest() failed to account for the fact that for
> remote vCPU-s dumping gets done through a pointer obtained from
> map_domain_page(): __get_guest() arranges for (apparent) accesses to
> hypervisor space to cause #GP(0).
> 
> Fixes: 6a1d72d3739e ('x86: split __{get,put}_user() into "guest" and "unsafe" variants')
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Using get_unsafe() might be an option as well, instead of the added
> extra conditional.

A third option might be to place them in a non Xen private VA range,
like we do for the compat argument translation. That's however too
much IMO.

> 
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -275,7 +275,9 @@ static void compat_show_guest_stack(stru
>      {
>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>              break;
> -        if ( __get_guest(addr, stack) )
> +        if ( stack_page )
> +            addr = *stack;
> +        else if ( __get_guest(addr, stack) )
>          {
>              if ( i != 0 )
>                  printk("\n    ");
> @@ -344,7 +346,9 @@ static void show_guest_stack(struct vcpu
>      {
>          if ( (((long)stack - 1) ^ ((long)(stack + 1) - 1)) & mask )
>              break;
> -        if ( __get_guest(addr, stack) )
> +        if ( stack_page )
> +            addr = *stack;

I don't really have a strong opinion regarding this or the usage of
get_unsafe. When v == current we have the extra protection of being
sure no private Xen mappings are accessed, but that's only for a
single vCPU at most, at which point we might just use get_unsafe
uniformly. I guess I have a slight preference for get_unsafe because
using get_guest doesn't really get us anything.

If you keep it in the current form, or decide to switch to
get_unsafe:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.