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
