* [PATCH] x86/traps: fix an off-by-one error
@ 2020-05-05 11:06 Hongyan Xia
2020-05-05 13:38 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Hongyan Xia @ 2020-05-05 11:06 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: fix an off-by-one error
2020-05-05 11:06 [PATCH] x86/traps: fix an off-by-one error Hongyan Xia
@ 2020-05-05 13:38 ` Jan Beulich
2020-05-05 14:01 ` Hongyan Xia
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2020-05-05 13:38 UTC (permalink / raw)
To: Hongyan Xia; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: fix an off-by-one error
2020-05-05 13:38 ` Jan Beulich
@ 2020-05-05 14:01 ` Hongyan Xia
2020-05-05 14:55 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Hongyan Xia @ 2020-05-05 14:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/traps: fix an off-by-one error
2020-05-05 14:01 ` Hongyan Xia
@ 2020-05-05 14:55 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2020-05-05 14:55 UTC (permalink / raw)
To: Hongyan Xia; +Cc: xen-devel, Roger Pau Monné, Wei Liu, Andrew Cooper
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-05 14:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 11:06 [PATCH] x86/traps: fix an off-by-one error Hongyan Xia
2020-05-05 13:38 ` Jan Beulich
2020-05-05 14:01 ` Hongyan Xia
2020-05-05 14:55 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.