Re: [PATCH 07/12] x86/compat: get_gate_vma should check for both 32-bit compat and x32

[Andy Lutomirski]

On Mon, Jun 22, 2015 at 4:55 AM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
> Change this to CONFIG_COMPAT so both 32-bit compat and x32 will do the
> check.
>
> Signed-off-by: Brian Gerst <brgerst@xxxxxxxxx>
> ---
>  arch/x86/entry/vsyscall/vsyscall_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
> index 2dcc6ff..26a46f4 100644
> --- a/arch/x86/entry/vsyscall/vsyscall_64.c
> +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
> @@ -290,7 +290,7 @@ static struct vm_area_struct gate_vma = {
>
>  struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
>  {
> -#ifdef CONFIG_IA32_EMULATION
> +#ifdef CONFIG_COMPAT
>         if (!mm || mm->context.ia32_compat)
>                 return NULL;
>  #endif


This makes little sense to me.

First, why is the !mm check guarded by any ifdef at all?  If this said
"if (mm && mm->...)", it would make no sense.

Second, and more importantly, what does mm->context.ia32_compat mean
in the new less-nonsensical regime?  The flag itself is defined in a
way that makes no sense (it's either 0, TIF_X32, or TIF_IA32 --
presumably it should be an enum).  There aren't a whole lot of things
that care -- it's just this check and some uprobe thing.  At some
point, mpx will start caring, too.  There's also TIF_ADDR32, which is
similarly ridiculous (why isn't it part of mm->context?, and why does
it exist at all).

I think that the questions we want to be able to answer are:

1. Is this mm intended to be addressable using 32 bits?  If so, we
should probably not show the vsyscall area in /proc/self/maps.

2. Is this mm's mpx context intended to be used by 32-bit userspace?
(That's real 32 bit, not x32 -- x32 is certainly 64-bit as far as MPX
is concerned.)

3. Is the current mmap call intended to return something in the low 32
bits?  Presumably that should depend only on the mmap call's bitness
(is_compat_task, etc, which we really need to rename to something like
in_compat_syscall).

I find myself wondering whether there is any legitimate reason that
TASK_SIZE isn't the same thing as TASK_SIZE_MAX...

Anyway, your patch is probably fine -- you're not actually making
anything worse.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/