compel: Do not loose sign of result in compat syscall

Submitted by Cyrill Gorcunov on Oct. 30, 2017, 8:22 p.m.

Details

Message ID 20171030202214.GA2001@uranus.lan
State New
Series "compel: Do not loose sign of result in compat syscall"
Headers show

Commit Message

Cyrill Gorcunov Oct. 30, 2017, 8:22 p.m.
On Mon, Oct 30, 2017 at 01:14:13PM -0700, Andrey Vagin wrote:
> > 
> > This code has nothing to do with such issues, it's up to a caller
> > to verify the values obtained.
> 
> This code modifies a valid value, so you fix one issue and create a new
> one. Could you fix both of them? ;)

It doesn't create new one but rather reveal arhitecture problem,
which I missed in first place, don't you see? For vz7 instance
the additional fix is. For vanilla instance i will merge them
into one, hopefully tomorrow.
---
From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
Date: Mon, 30 Oct 2017 23:16:58 +0300
Subject: [PATCH] compel: x86,compat -- Make sure mmap fits compat task size

In addition to

 | commit 702d51315bdd337b9ab3d32c952bb8a150440d45
 | Author: Cyrill Gorcunov <gorcunov@virtuozzo.com>
 |
 |     compel: Do not loose sign of result in compat syscall

It's due to compel interface which operates inside ia-32e mode
where we have to keep sign of syscall up to a caller layer.

Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
---
 compel/arch/x86/src/lib/infect.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
index ac5f8b0..e546ee3 100644
--- a/compel/arch/x86/src/lib/infect.c
+++ b/compel/arch/x86/src/lib/infect.c
@@ -348,6 +348,13 @@  void *remote_mmap(struct parasite_ctl *ctl,
 		return NULL;
 	}
 
+	/*
+	 * For compat tasks the address in foreign process
+	 * must lay inside 4 bytes.
+	 */
+	if (compat_task)
+		map &= 0xfffffffful;
+
 	return (void *)map;
 }
 

Comments

Dmitry Safonov Oct. 31, 2017, 11:47 a.m.
2017-10-30 20:22 GMT+00:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> On Mon, Oct 30, 2017 at 01:14:13PM -0700, Andrey Vagin wrote:
>> >
>> > This code has nothing to do with such issues, it's up to a caller
>> > to verify the values obtained.
>>
>> This code modifies a valid value, so you fix one issue and create a new
>> one. Could you fix both of them? ;)
>
> It doesn't create new one but rather reveal arhitecture problem,
> which I missed in first place, don't you see? For vz7 instance
> the additional fix is. For vanilla instance i will merge them
> into one, hopefully tomorrow.
> ---
> From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> Date: Mon, 30 Oct 2017 23:16:58 +0300
> Subject: [PATCH] compel: x86,compat -- Make sure mmap fits compat task size
>
> In addition to
>
>  | commit 702d51315bdd337b9ab3d32c952bb8a150440d45
>  | Author: Cyrill Gorcunov <gorcunov@virtuozzo.com>
>  |
>  |     compel: Do not loose sign of result in compat syscall
>
> It's due to compel interface which operates inside ia-32e mode
> where we have to keep sign of syscall up to a caller layer.
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> ---
>  compel/arch/x86/src/lib/infect.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> index ac5f8b0..e546ee3 100644
> --- a/compel/arch/x86/src/lib/infect.c
> +++ b/compel/arch/x86/src/lib/infect.c
> @@ -348,6 +348,13 @@ void *remote_mmap(struct parasite_ctl *ctl,
>                 return NULL;
>         }
>
> +       /*
> +        * For compat tasks the address in foreign process
> +        * must lay inside 4 bytes.
> +        */
> +       if (compat_task)
> +               map &= 0xfffffffful;

A nit:
TASK_SIZE_IA32?
Cyrill Gorcunov Oct. 31, 2017, 11:53 a.m.
On Tue, Oct 31, 2017 at 11:47:32AM +0000, Dmitry Safonov wrote:
> > From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > Date: Mon, 30 Oct 2017 23:16:58 +0300
> > Subject: [PATCH] compel: x86,compat -- Make sure mmap fits compat task size
> >
> > In addition to
> >
> >  | commit 702d51315bdd337b9ab3d32c952bb8a150440d45
> >  | Author: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> >  |
> >  |     compel: Do not loose sign of result in compat syscall
> >
> > It's due to compel interface which operates inside ia-32e mode
> > where we have to keep sign of syscall up to a caller layer.
> >
> > Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
> > ---
> >  compel/arch/x86/src/lib/infect.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
> > index ac5f8b0..e546ee3 100644
> > --- a/compel/arch/x86/src/lib/infect.c
> > +++ b/compel/arch/x86/src/lib/infect.c
> > @@ -348,6 +348,13 @@ void *remote_mmap(struct parasite_ctl *ctl,
> >                 return NULL;
> >         }
> >
> > +       /*
> > +        * For compat tasks the address in foreign process
> > +        * must lay inside 4 bytes.
> > +        */
> > +       if (compat_task)
> > +               map &= 0xfffffffful;
> 
> A nit:
> TASK_SIZE_IA32?

Not exactly. We could generate a mask from it but I think it's not that required,
at least by now.
Dmitry Safonov Oct. 31, 2017, 12:08 p.m.
2017-10-31 11:53 GMT+00:00 Cyrill Gorcunov <gorcunov@virtuozzo.com>:
> On Tue, Oct 31, 2017 at 11:47:32AM +0000, Dmitry Safonov wrote:
>> > From: Cyrill Gorcunov <gorcunov@virtuozzo.com>
>> > Date: Mon, 30 Oct 2017 23:16:58 +0300
>> > Subject: [PATCH] compel: x86,compat -- Make sure mmap fits compat task size
>> >
>> > In addition to
>> >
>> >  | commit 702d51315bdd337b9ab3d32c952bb8a150440d45
>> >  | Author: Cyrill Gorcunov <gorcunov@virtuozzo.com>
>> >  |
>> >  |     compel: Do not loose sign of result in compat syscall
>> >
>> > It's due to compel interface which operates inside ia-32e mode
>> > where we have to keep sign of syscall up to a caller layer.
>> >
>> > Signed-off-by: Cyrill Gorcunov <gorcunov@virtuozzo.com>
>> > ---
>> >  compel/arch/x86/src/lib/infect.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/compel/arch/x86/src/lib/infect.c b/compel/arch/x86/src/lib/infect.c
>> > index ac5f8b0..e546ee3 100644
>> > --- a/compel/arch/x86/src/lib/infect.c
>> > +++ b/compel/arch/x86/src/lib/infect.c
>> > @@ -348,6 +348,13 @@ void *remote_mmap(struct parasite_ctl *ctl,
>> >                 return NULL;
>> >         }
>> >
>> > +       /*
>> > +        * For compat tasks the address in foreign process
>> > +        * must lay inside 4 bytes.
>> > +        */
>> > +       if (compat_task)
>> > +               map &= 0xfffffffful;
>>
>> A nit:
>> TASK_SIZE_IA32?
>
> Not exactly. We could generate a mask from it but I think it's not that required,
> at least by now.

Wait, I kind of don't get it:
How map can be > 4 bytes if you get it from (user_regs_struct32::ax), which is
4 bytes (u32) for compat tasks?
We may insert BUG_ON(map & 0xffffffff00000000) or something.
Cyrill Gorcunov Oct. 31, 2017, 12:33 p.m.
On Tue, Oct 31, 2017 at 12:08:11PM +0000, Dmitry Safonov wrote:
> >
> > Not exactly. We could generate a mask from it but I think it's not that required,
> > at least by now.
> 
> Wait, I kind of don't get it:
> How map can be > 4 bytes if you get it from (user_regs_struct32::ax), which is
> 4 bytes (u32) for compat tasks?
> We may insert BUG_ON(map & 0xffffffff00000000) or something.

Because we convert uint -> int -> long, and the sing of int get
propagated to upper bits. The map can't be more than 4 bytes but
we use signed long to keep the result.
Dmitry Safonov Oct. 31, 2017, 12:43 p.m.
2017-10-31 12:33 GMT+00:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> On Tue, Oct 31, 2017 at 12:08:11PM +0000, Dmitry Safonov wrote:
>> >
>> > Not exactly. We could generate a mask from it but I think it's not that required,
>> > at least by now.
>>
>> Wait, I kind of don't get it:
>> How map can be > 4 bytes if you get it from (user_regs_struct32::ax), which is
>> 4 bytes (u32) for compat tasks?
>> We may insert BUG_ON(map & 0xffffffff00000000) or something.
>
> Because we convert uint -> int -> long, and the sing of int get
> propagated to upper bits. The map can't be more than 4 bytes but
> we use signed long to keep the result.

Ok, I see, the sign extension.
It's not a very lovely solution but should work.
Cyrill Gorcunov Oct. 31, 2017, 12:58 p.m.
On Tue, Oct 31, 2017 at 12:43:04PM +0000, Dmitry Safonov wrote:
> 2017-10-31 12:33 GMT+00:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> > On Tue, Oct 31, 2017 at 12:08:11PM +0000, Dmitry Safonov wrote:
> >> >
> >> > Not exactly. We could generate a mask from it but I think it's not that required,
> >> > at least by now.
> >>
> >> Wait, I kind of don't get it:
> >> How map can be > 4 bytes if you get it from (user_regs_struct32::ax), which is
> >> 4 bytes (u32) for compat tasks?
> >> We may insert BUG_ON(map & 0xffffffff00000000) or something.
> >
> > Because we convert uint -> int -> long, and the sing of int get
> > propagated to upper bits. The map can't be more than 4 bytes but
> > we use signed long to keep the result.
> 
> Ok, I see, the sign extension.
> It's not a very lovely solution but should work.

We didn't merge any into vanilla criu yet, so if you have a better
idea -- you are more than welcome!
Dmitry Safonov Oct. 31, 2017, 1:14 p.m.
2017-10-31 12:58 GMT+00:00 Cyrill Gorcunov <gorcunov@gmail.com>:
> On Tue, Oct 31, 2017 at 12:43:04PM +0000, Dmitry Safonov wrote:
>> 2017-10-31 12:33 GMT+00:00 Cyrill Gorcunov <gorcunov@gmail.com>:
>> > On Tue, Oct 31, 2017 at 12:08:11PM +0000, Dmitry Safonov wrote:
>> >> >
>> >> > Not exactly. We could generate a mask from it but I think it's not that required,
>> >> > at least by now.
>> >>
>> >> Wait, I kind of don't get it:
>> >> How map can be > 4 bytes if you get it from (user_regs_struct32::ax), which is
>> >> 4 bytes (u32) for compat tasks?
>> >> We may insert BUG_ON(map & 0xffffffff00000000) or something.
>> >
>> > Because we convert uint -> int -> long, and the sing of int get
>> > propagated to upper bits. The map can't be more than 4 bytes but
>> > we use signed long to keep the result.
>>
>> Ok, I see, the sign extension.
>> It's not a very lovely solution but should work.
>
> We didn't merge any into vanilla criu yet, so if you have a better
> idea -- you are more than welcome!

I know, I didn't mean to say that your idea is bad,
that looks like they are all a bit of ugly:

1. Check (map & 0xffffffff) - this one
2. The way it's done in mmap_bug_test(): (map % PAGE_SIZE)
3. Add IS_ERR_VALUE(x, compat) (the ugliest, IMHO)
Cyrill Gorcunov Oct. 31, 2017, 1:18 p.m.
On Tue, Oct 31, 2017 at 01:14:21PM +0000, Dmitry Safonov wrote:
> >>
> >> Ok, I see, the sign extension.
> >> It's not a very lovely solution but should work.
> >
> > We didn't merge any into vanilla criu yet, so if you have a better
> > idea -- you are more than welcome!
> 
> I know, I didn't mean to say that your idea is bad,
> that looks like they are all a bit of ugly:
> 
> 1. Check (map & 0xffffffff) - this one
> 2. The way it's done in mmap_bug_test(): (map % PAGE_SIZE)
> 3. Add IS_ERR_VALUE(x, compat) (the ugliest, IMHO)

Sure thing. And that's why I pointed that better ideas are
more than welcome :)