xen/arch/x86/traps.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
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
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
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
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
© 2016 - 2024 Red Hat, Inc.