1. Patchew
  2. Xen
  3. View series

[PATCH] x86/traps: fix an off-by-one error

Hongyan Xia posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/37b7ec049ff82f92cc6724a743867e1cd9365f5b.1588676790.git.hongyxia@amazon.com
Maintainers: Wei Liu <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>, Jan Beulich <jbeulich@suse.com>, Andrew Cooper <andrew.cooper3@citrix.com>
There is a newer version of this series
xen/arch/x86/traps.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[PATCH] x86/traps: fix an off-by-one error
Posted by Hongyan Xia 5 years, 9 months ago 
From: Hongyan Xia <hongyxia@amazon.com>

stack++ can go into the next page and unmap_domain_page() will unmap the
wrong one, causing mapcache and memory corruption. Fix.

This is found with direct map removal. For now, the idle domain does not
have a mapcache and uses the direct map, so no errors will occur.

Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/traps.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 33e5d21ece..f033a804a3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -300,6 +300,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
     int i;
     unsigned long *stack, addr;
     unsigned long mask = STACK_SIZE;
+    void *stack_page = NULL;
 
     /* Avoid HVM as we don't know what the stack looks like. */
     if ( is_hvm_vcpu(v) )
@@ -328,7 +329,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
         vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
         if ( !vcpu )
         {
-            stack = do_page_walk(v, (unsigned long)stack);
+            stack_page = stack = do_page_walk(v, (unsigned long)stack);
             if ( (unsigned long)stack < PAGE_SIZE )
             {
                 printk("Inaccessible guest memory.\n");
@@ -358,7 +359,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
     if ( mask == PAGE_SIZE )
     {
         BUILD_BUG_ON(PAGE_SIZE == STACK_SIZE);
-        unmap_domain_page(stack);
+        unmap_domain_page(stack_page);
     }
     if ( i == 0 )
         printk("Stack empty.");
-- 
2.17.1
Re: [PATCH] x86/traps: fix an off-by-one error
Posted by Jan Beulich 5 years, 9 months ago 
On 05.05.2020 13:06, Hongyan Xia wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -300,6 +300,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
>      int i;
>      unsigned long *stack, addr;
>      unsigned long mask = STACK_SIZE;
> +    void *stack_page = NULL;
>  
>      /* Avoid HVM as we don't know what the stack looks like. */
>      if ( is_hvm_vcpu(v) )
> @@ -328,7 +329,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
>          vcpu = maddr_get_owner(read_cr3()) == v->domain ? v : NULL;
>          if ( !vcpu )
>          {
> -            stack = do_page_walk(v, (unsigned long)stack);
> +            stack_page = stack = do_page_walk(v, (unsigned long)stack);
>              if ( (unsigned long)stack < PAGE_SIZE )
>              {
>                  printk("Inaccessible guest memory.\n");
> @@ -358,7 +359,7 @@ static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
>      if ( mask == PAGE_SIZE )
>      {
>          BUILD_BUG_ON(PAGE_SIZE == STACK_SIZE);
> -        unmap_domain_page(stack);
> +        unmap_domain_page(stack_page);
>      }

With this I think you want to change the whole construct here to

    if ( stack_page )
        unmap_domain_page(stack_page);

i.e. with the then no longer relevant BUILD_BUG_ON() also dropped.

What's more important though - please also fix the same issue in
compat_show_guest_stack(). Unless I'm mistaken of course, in which
case it would be nice if the description could mention why the
other similar function isn't affected.

Jan
Re: [PATCH] x86/traps: fix an off-by-one error
Posted by Hongyan Xia 5 years, 9 months ago 
On Tue, 2020-05-05 at 15:38 +0200, Jan Beulich wrote:
> On 05.05.2020 13:06, Hongyan Xia wrote:
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -300,6 +300,7 @@ static void show_guest_stack(struct vcpu *v,
> > const struct cpu_user_regs *regs)
> >      int i;
> >      unsigned long *stack, addr;
> >      unsigned long mask = STACK_SIZE;
> > +    void *stack_page = NULL;
> >  
> >      /* Avoid HVM as we don't know what the stack looks like. */
> >      if ( is_hvm_vcpu(v) )
> > @@ -328,7 +329,7 @@ static void show_guest_stack(struct vcpu *v,
> > const struct cpu_user_regs *regs)
> >          vcpu = maddr_get_owner(read_cr3()) == v->domain ? v :
> > NULL;
> >          if ( !vcpu )
> >          {
> > -            stack = do_page_walk(v, (unsigned long)stack);
> > +            stack_page = stack = do_page_walk(v, (unsigned
> > long)stack);
> >              if ( (unsigned long)stack < PAGE_SIZE )
> >              {
> >                  printk("Inaccessible guest memory.\n");
> > @@ -358,7 +359,7 @@ static void show_guest_stack(struct vcpu *v,
> > const struct cpu_user_regs *regs)
> >      if ( mask == PAGE_SIZE )
> >      {
> >          BUILD_BUG_ON(PAGE_SIZE == STACK_SIZE);
> > -        unmap_domain_page(stack);
> > +        unmap_domain_page(stack_page);
> >      }
> 
> With this I think you want to change the whole construct here to
> 
>     if ( stack_page )
>         unmap_domain_page(stack_page);
> 
> i.e. with the then no longer relevant BUILD_BUG_ON() also dropped.

I wonder if such a construct is better with UNMAP_DOMAIN_PAGE(), since
it deals with NULL and will nullify it to prevent misuse.

> What's more important though - please also fix the same issue in
> compat_show_guest_stack(). Unless I'm mistaken of course, in which
> case it would be nice if the description could mention why the
> other similar function isn't affected.

Compat suffers from the same problem. Thanks for pointing that out.

Hongyan
Re: [PATCH] x86/traps: fix an off-by-one error
Posted by Jan Beulich 5 years, 9 months ago 
On 05.05.2020 16:01, Hongyan Xia wrote:
> On Tue, 2020-05-05 at 15:38 +0200, Jan Beulich wrote:
>> On 05.05.2020 13:06, Hongyan Xia wrote:
>>> @@ -358,7 +359,7 @@ static void show_guest_stack(struct vcpu *v,
>>> const struct cpu_user_regs *regs)
>>>      if ( mask == PAGE_SIZE )
>>>      {
>>>          BUILD_BUG_ON(PAGE_SIZE == STACK_SIZE);
>>> -        unmap_domain_page(stack);
>>> +        unmap_domain_page(stack_page);
>>>      }
>>
>> With this I think you want to change the whole construct here to
>>
>>     if ( stack_page )
>>         unmap_domain_page(stack_page);
>>
>> i.e. with the then no longer relevant BUILD_BUG_ON() also dropped.
> 
> I wonder if such a construct is better with UNMAP_DOMAIN_PAGE(), since
> it deals with NULL and will nullify it to prevent misuse.

In the case here I think I agree. For the future may I ask that
you wait with sending a new version until the discussion on the
previous one has settled?

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.