Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1301600 > unrolled thread

Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate  table (in entry.S) to use it Yury Norov <ynorov@caviumnetworks.com> - 2016-01-05 16:30 +0100
    Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it Arnd Bergmann <arnd@arndb.de> - 2016-01-05 22:20 +0100
      Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate  table (in entry.S) to use it Catalin Marinas <catalin.marinas@arm.com> - 2016-01-06 18:20 +0100
        Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it Arnd Bergmann <arnd@arndb.de> - 2016-01-07 15:20 +0100
          Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate  table (in entry.S) to use it Yury Norov <ynorov@caviumnetworks.com> - 2016-01-07 16:50 +0100
          Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate  table (in entry.S) to use it Catalin Marinas <catalin.marinas@arm.com> - 2016-01-07 18:30 +0100

#1301600 — Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

On Thu, Dec 17, 2015 at 09:50:52PM +0100, Arnd Bergmann wrote:
> On Thursday 17 December 2015 12:14:20 Andrew Pinski wrote:
> > On Thu, Dec 17, 2015 at 12:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thursday 17 December 2015 18:27:53 Catalin Marinas wrote:
> > >> On Wed, Dec 16, 2015 at 12:42:38AM +0300, Yury Norov wrote:
> > >
> > >> > +#define compat_sys_lookup_dcookie      sys_lookup_dcookie
> > >> > +#define compat_sys_pread64             sys_pread64
> > >> > +#define compat_sys_pwrite64            sys_pwrite64
> > >> > +#define compat_sys_readahead           sys_readahead
> > >> > +#define compat_sys_shmat               sys_shmat
> > >>
> > >> I wonder whether we need wrappers (actually, not only for these but
> > >> sys_read etc.). These functions take either a pointer or a size_t
> > >> argument which are 32-bit with ILP32 but treated as 64-bit by an LP64
> > >> kernel. Can we guarantee that user space zeros the top 32-bit of the
> > >> arguments passed here?
> > >
> > > I'm pretty sure that is safe. I haven't read the calling conventions
> > > specification for arm64 ilp32, but usually all function arguments are
> > > passed as 64-bit registers with proper sign-extend or zero-extend.
> > 
> > Well (just like LP64 on AARCH64), when passing a 32bit value to a
> > function, the upper 32bits are undefined.  I ran into this when I was
> > debugging the GCC go library on ILP32 (though reproduced with pure C
> > code) and the assembly functions inside glibc where pointers are
> > passed with the upper 32bits as undefined.
> > So we have an issue if called with syscall function or using pure
> > assembly to create the syscall functions (which glibc does).
> 
> Ok, I see :-(
> 
> So the calling conventions avoid the problem of being able to set
> the upper bits from malicious user space when the kernel assumes they
> are zeroed out (we had security bugs in this area, before we introduced
> SYSCALL_DEFINEx()), but it means that we need wrappers around each
> syscall that takes an argument that is different length between user
> and kernel space (as Catalin guessed). arch/s390 has the same problem and
> works around it with code in arch/s390/kernel/compat_wrapper.c, while
> other architectures (at least powerpc, x86 and tile IIRC, don't know much
> about mips, parisc and sparc) don't have the problem because of their
> calling conventions.
> 
> This also means that we cannot work around it in glibc at all, because
> we have to be able to handle malicious user space, so it has to be
> done in the kernel using something similar to what s390 does.
> 
> 	Arnd

So it seems like we (should) have 2 compat modes - with and without access
to upper half of register. I'm thinking now on how put it in generic
unistd.h less painfull way.

Beside of that, I think I almost finished with all current comments. As
this issue is not related to ILP32 directly, I think, it's better to show
it now, as there is pretty massive rework. What do you think?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302204 — Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

On Tuesday 05 January 2016 18:26:57 Yury Norov wrote:
> > So the calling conventions avoid the problem of being able to set
> > the upper bits from malicious user space when the kernel assumes they
> > are zeroed out (we had security bugs in this area, before we introduced
> > SYSCALL_DEFINEx()), but it means that we need wrappers around each
> > syscall that takes an argument that is different length between user
> > and kernel space (as Catalin guessed). arch/s390 has the same problem and
> > works around it with code in arch/s390/kernel/compat_wrapper.c, while
> > other architectures (at least powerpc, x86 and tile IIRC, don't know much
> > about mips, parisc and sparc) don't have the problem because of their
> > calling conventions.
> > 
> > This also means that we cannot work around it in glibc at all, because
> > we have to be able to handle malicious user space, so it has to be
> > done in the kernel using something similar to what s390 does.
> > 
> >       Arnd
> 
> So it seems like we (should) have 2 compat modes - with and without access
> to upper half of register. I'm thinking now on how put it in generic
> unistd.h less painfull way.

I think we can do that by slightly modifying the existing __SYSCALL/__SC_3264/
__SC_COMP/__SC_COMP_3264 macros: The first two need extra wrappers for
arm64-ilp32 and s390, the other two don't.

We can use some clever string concatenation to add a ##_wrapper to the name
of the handler where needed and then just have a file that implements
the wrappers, copied from s390.

Unfortunately, we can't just zero out all the upper halves and be done with
it: even if we went back to passing 64-bit arguments as separate 32-bit
registers, we'd still need to deal with sign-extending negative 32-bit
numbers.

> Beside of that, I think I almost finished with all current comments. As
> this issue is not related to ILP32 directly, I think, it's better to show
> it now, as there is pretty massive rework. What do you think?

Good idea, yes.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302907

On Tue, Jan 05, 2016 at 10:12:20PM +0100, Arnd Bergmann wrote:
> On Tuesday 05 January 2016 18:26:57 Yury Norov wrote:
> > > So the calling conventions avoid the problem of being able to set
> > > the upper bits from malicious user space when the kernel assumes they
> > > are zeroed out (we had security bugs in this area, before we introduced
> > > SYSCALL_DEFINEx()), but it means that we need wrappers around each
> > > syscall that takes an argument that is different length between user
> > > and kernel space (as Catalin guessed). arch/s390 has the same problem and
> > > works around it with code in arch/s390/kernel/compat_wrapper.c, while
> > > other architectures (at least powerpc, x86 and tile IIRC, don't know much
> > > about mips, parisc and sparc) don't have the problem because of their
> > > calling conventions.
> > > 
> > > This also means that we cannot work around it in glibc at all, because
> > > we have to be able to handle malicious user space, so it has to be
> > > done in the kernel using something similar to what s390 does.
> > 
> > So it seems like we (should) have 2 compat modes - with and without access
> > to upper half of register. I'm thinking now on how put it in generic
> > unistd.h less painfull way.
> 
> I think we can do that by slightly modifying the existing __SYSCALL/__SC_3264/
> __SC_COMP/__SC_COMP_3264 macros: The first two need extra wrappers for
> arm64-ilp32 and s390, the other two don't.
> 
> We can use some clever string concatenation to add a ##_wrapper to the name
> of the handler where needed and then just have a file that implements
> the wrappers, copied from s390.
> 
> Unfortunately, we can't just zero out all the upper halves and be done with
> it: even if we went back to passing 64-bit arguments as separate 32-bit
> registers, we'd still need to deal with sign-extending negative 32-bit
> numbers.

How many syscalls would we need sign-extension for? Most are probably
already handled by specific compat_sys_* functions, otherwise A32 compat
wouldn't work properly.

Anyway, I think we can get away with not modifying the generic __SYSCALL
definition and only use something like
arch/s390/kernel/compat_wrapper.c. In sys_ilp32.c, we would make
__SYSCALL expand the function name with some ilp32_ prefix.

For existing compat_* syscalls, we only need to handle the pointer types
(something like the s390's __TYPE_IS_PTR). I think other types are
already handled by defining the prototype with compat_ulong_t etc.

For native syscalls like sys_read, apart from pointers we also need to
handle size_t. The wrapper would need to be defined using compat types:

ILP32_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, compat_size_t, count)

and let the compiler handle the conversion to size_t automatically when
calling sys_read from the wrapper.

> > Beside of that, I think I almost finished with all current comments. As
> > this issue is not related to ILP32 directly, I think, it's better to show
> > it now, as there is pretty massive rework. What do you think?
> 
> Good idea, yes.

Note that we still need to sort the 0/sign extension out before we
"declare" the ILP32 ABI stable.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303603 — Re: [PATCH v6 12/20] arm64:ilp32: add sys_ilp32.c and a separate table (in entry.S) to use it

On Wednesday 06 January 2016 17:10:47 Catalin Marinas wrote:
> On Tue, Jan 05, 2016 at 10:12:20PM +0100, Arnd Bergmann wrote:
> > On Tuesday 05 January 2016 18:26:57 Yury Norov wrote:
> > > > So the calling conventions avoid the problem of being able to set
> > > > the upper bits from malicious user space when the kernel assumes they
> > > > are zeroed out (we had security bugs in this area, before we introduced
> > > > SYSCALL_DEFINEx()), but it means that we need wrappers around each
> > > > syscall that takes an argument that is different length between user
> > > > and kernel space (as Catalin guessed). arch/s390 has the same problem and
> > > > works around it with code in arch/s390/kernel/compat_wrapper.c, while
> > > > other architectures (at least powerpc, x86 and tile IIRC, don't know much
> > > > about mips, parisc and sparc) don't have the problem because of their
> > > > calling conventions.
> > > > 
> > > > This also means that we cannot work around it in glibc at all, because
> > > > we have to be able to handle malicious user space, so it has to be
> > > > done in the kernel using something similar to what s390 does.
> > > 
> > > So it seems like we (should) have 2 compat modes - with and without access
> > > to upper half of register. I'm thinking now on how put it in generic
> > > unistd.h less painfull way.
> > 
> > I think we can do that by slightly modifying the existing __SYSCALL/__SC_3264/
> > __SC_COMP/__SC_COMP_3264 macros: The first two need extra wrappers for
> > arm64-ilp32 and s390, the other two don't.
> > 
> > We can use some clever string concatenation to add a ##_wrapper to the name
> > of the handler where needed and then just have a file that implements
> > the wrappers, copied from s390.
> > 
> > Unfortunately, we can't just zero out all the upper halves and be done with
> > it: even if we went back to passing 64-bit arguments as separate 32-bit
> > registers, we'd still need to deal with sign-extending negative 32-bit
> > numbers.
> 
> How many syscalls would we need sign-extension for? Most are probably
> already handled by specific compat_sys_* functions, otherwise A32 compat
> wouldn't work properly.

Good point. I suppose any system call that expects a negative argument
may run into this on all architectures and require a COMPAT_SYSCALL handler,
but only s390 cares about doing the extension for the entire set of syscalls.

This may be to work around a peculiarity of s390, which has now two
but three possible 32-to-64 extension modes: signed int, unsigned int
and pointer. The third one sets the top 33 bits to zero, clearing the
top bit of the 31-bit pointer value in the process. Nothing else needs
this, so if we just clear the upper bits on all system calls and go
back to passing 64-bit arguments as pairs, we are fine and have a much
simpler solution.

> Anyway, I think we can get away with not modifying the generic __SYSCALL
> definition and only use something like
> arch/s390/kernel/compat_wrapper.c. In sys_ilp32.c, we would make
> __SYSCALL expand the function name with some ilp32_ prefix.

I couldn't think of a way, but I'm gladly proven wrong here.

> For existing compat_* syscalls, we only need to handle the pointer types
> (something like the s390's __TYPE_IS_PTR). I think other types are
> already handled by defining the prototype with compat_ulong_t etc.

Right.
 
> For native syscalls like sys_read, apart from pointers we also need to
> handle size_t. The wrapper would need to be defined using compat types:
> 
> ILP32_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, compat_size_t, count)
> 
> and let the compiler handle the conversion to size_t automatically when
> calling sys_read from the wrapper.

Correct. I don't think we need an ILP32_SYSCALL_DEFINEx set of macros
though, the existing COMPAT_SYSCALL_DEFINEx ones should get this right
already.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303686

On Thu, Jan 07, 2016 at 03:13:37PM +0100, Arnd Bergmann wrote:
> On Wednesday 06 January 2016 17:10:47 Catalin Marinas wrote:
> > On Tue, Jan 05, 2016 at 10:12:20PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 05 January 2016 18:26:57 Yury Norov wrote:
> > > > > So the calling conventions avoid the problem of being able to set
> > > > > the upper bits from malicious user space when the kernel assumes they
> > > > > are zeroed out (we had security bugs in this area, before we introduced
> > > > > SYSCALL_DEFINEx()), but it means that we need wrappers around each
> > > > > syscall that takes an argument that is different length between user
> > > > > and kernel space (as Catalin guessed). arch/s390 has the same problem and
> > > > > works around it with code in arch/s390/kernel/compat_wrapper.c, while
> > > > > other architectures (at least powerpc, x86 and tile IIRC, don't know much
> > > > > about mips, parisc and sparc) don't have the problem because of their
> > > > > calling conventions.
> > > > > 
> > > > > This also means that we cannot work around it in glibc at all, because
> > > > > we have to be able to handle malicious user space, so it has to be
> > > > > done in the kernel using something similar to what s390 does.
> > > > 
> > > > So it seems like we (should) have 2 compat modes - with and without access
> > > > to upper half of register. I'm thinking now on how put it in generic
> > > > unistd.h less painfull way.
> > > 
> > > I think we can do that by slightly modifying the existing __SYSCALL/__SC_3264/
> > > __SC_COMP/__SC_COMP_3264 macros: The first two need extra wrappers for
> > > arm64-ilp32 and s390, the other two don't.
> > > 
> > > We can use some clever string concatenation to add a ##_wrapper to the name
> > > of the handler where needed and then just have a file that implements
> > > the wrappers, copied from s390.
> > > 
> > > Unfortunately, we can't just zero out all the upper halves and be done with
> > > it: even if we went back to passing 64-bit arguments as separate 32-bit
> > > registers, we'd still need to deal with sign-extending negative 32-bit
> > > numbers.
> > 
> > How many syscalls would we need sign-extension for? Most are probably
> > already handled by specific compat_sys_* functions, otherwise A32 compat
> > wouldn't work properly.
> 
> Good point. I suppose any system call that expects a negative argument
> may run into this on all architectures and require a COMPAT_SYSCALL handler,
> but only s390 cares about doing the extension for the entire set of syscalls.
> 
> This may be to work around a peculiarity of s390, which has now two
> but three possible 32-to-64 extension modes: signed int, unsigned int
> and pointer. The third one sets the top 33 bits to zero, clearing the
> top bit of the 31-bit pointer value in the process. Nothing else needs
> this, so if we just clear the upper bits on all system calls and go
> back to passing 64-bit arguments as pairs, we are fine and have a much
> simpler solution.

Wrappers will not add extra complexity because we already have it in
s390 port. In other hand, splitting and then assembling 64-bit values
affects performance so small, that we may not care about it much.

So, both approaches are acceptable for me. But I'd choose wrappers,
because it looks more generic, and more fun. :)

This way we can make something like ILP16 easily (can't imagine what
for, though).

> 
> > Anyway, I think we can get away with not modifying the generic __SYSCALL
> > definition and only use something like
> > arch/s390/kernel/compat_wrapper.c. In sys_ilp32.c, we would make
> > __SYSCALL expand the function name with some ilp32_ prefix.
> 
> I couldn't think of a way, but I'm gladly proven wrong here.
> 
> > For existing compat_* syscalls, we only need to handle the pointer types
> > (something like the s390's __TYPE_IS_PTR). I think other types are
> > already handled by defining the prototype with compat_ulong_t etc.
> 
> Right.
>  
> > For native syscalls like sys_read, apart from pointers we also need to
> > handle size_t. The wrapper would need to be defined using compat types:
> > 
> > ILP32_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, compat_size_t, count)
> > 
> > and let the compiler handle the conversion to size_t automatically when
> > calling sys_read from the wrapper.
> 
> Correct. I don't think we need an ILP32_SYSCALL_DEFINEx set of macros
> though, the existing COMPAT_SYSCALL_DEFINEx ones should get this right
> already.
> 
> 	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1303750

On Thu, Jan 07, 2016 at 03:13:37PM +0100, Arnd Bergmann wrote:
> On Wednesday 06 January 2016 17:10:47 Catalin Marinas wrote:
> > On Tue, Jan 05, 2016 at 10:12:20PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 05 January 2016 18:26:57 Yury Norov wrote:
> > > > > So the calling conventions avoid the problem of being able to set
> > > > > the upper bits from malicious user space when the kernel assumes they
> > > > > are zeroed out (we had security bugs in this area, before we introduced
> > > > > SYSCALL_DEFINEx()), but it means that we need wrappers around each
> > > > > syscall that takes an argument that is different length between user
> > > > > and kernel space (as Catalin guessed). arch/s390 has the same problem and
> > > > > works around it with code in arch/s390/kernel/compat_wrapper.c, while
> > > > > other architectures (at least powerpc, x86 and tile IIRC, don't know much
> > > > > about mips, parisc and sparc) don't have the problem because of their
> > > > > calling conventions.
> > > > > 
> > > > > This also means that we cannot work around it in glibc at all, because
> > > > > we have to be able to handle malicious user space, so it has to be
> > > > > done in the kernel using something similar to what s390 does.
> > > > 
> > > > So it seems like we (should) have 2 compat modes - with and without access
> > > > to upper half of register. I'm thinking now on how put it in generic
> > > > unistd.h less painfull way.
> > > 
> > > I think we can do that by slightly modifying the existing __SYSCALL/__SC_3264/
> > > __SC_COMP/__SC_COMP_3264 macros: The first two need extra wrappers for
> > > arm64-ilp32 and s390, the other two don't.
> > > 
> > > We can use some clever string concatenation to add a ##_wrapper to the name
> > > of the handler where needed and then just have a file that implements
> > > the wrappers, copied from s390.
> > > 
> > > Unfortunately, we can't just zero out all the upper halves and be done with
> > > it: even if we went back to passing 64-bit arguments as separate 32-bit
> > > registers, we'd still need to deal with sign-extending negative 32-bit
> > > numbers.
> > 
> > How many syscalls would we need sign-extension for? Most are probably
> > already handled by specific compat_sys_* functions, otherwise A32 compat
> > wouldn't work properly.
> 
> Good point. I suppose any system call that expects a negative argument
> may run into this on all architectures and require a COMPAT_SYSCALL handler,
> but only s390 cares about doing the extension for the entire set of syscalls.
> 
> This may be to work around a peculiarity of s390, which has now two
> but three possible 32-to-64 extension modes: signed int, unsigned int
> and pointer. The third one sets the top 33 bits to zero, clearing the
> top bit of the 31-bit pointer value in the process. Nothing else needs
> this, so if we just clear the upper bits on all system calls and go
> back to passing 64-bit arguments as pairs, we are fine and have a much
> simpler solution.

It would be indeed simpler from a kernel perspective. I'm not sure about
the performance impact (a wrapper which does "mov wn, wn" for the first
6 registers, on top of existing wrappers). OTOH, with explicit wrappers
we have the overhead of an additional function call, so we may be better
off with the former.

> > For native syscalls like sys_read, apart from pointers we also need to
> > handle size_t. The wrapper would need to be defined using compat types:
> > 
> > ILP32_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf, compat_size_t, count)
> > 
> > and let the compiler handle the conversion to size_t automatically when
> > calling sys_read from the wrapper.
> 
> Correct. I don't think we need an ILP32_SYSCALL_DEFINEx set of macros
> though, the existing COMPAT_SYSCALL_DEFINEx ones should get this right
> already.

The existing COMPAT_SYSCALL_DEFINEx macros generate the wrapper and
definition for the compat_sys_* functions. What I meant by an
ILP32_SYSCALL_DEFINEx is a macro which only generates a wrapper that
calls into the native syscall (after sanitizing the arguments).

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web