MT_HIGH_VECTOR mapping set read-only creating illegal access

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Michael Bohan]

Hi,

In arch/arm/kernel/traps.c:set_tls() and arch/arm/include/asm/tls.h, 
some configurations allow for a assignment of address 0xffff0ff0. This 
address falls within the MT_HIGH_VECTORS mapping setup in 
devicemaps_init(). That mapping is explicitly made read-only. Thus, the 
kernel takes a segfault when writing in set_tls().

It looks like this disparity may have been introduced in this commit:

commit 36bb94ba36f332de767cfaa3af6a5136435a3a9c
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Tue Nov 16 08:40:36 2010 +0000

     ARM: pgtable: provide RDONLY page table bit rather than WRITE bit

     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Is there a reason this mapping must be read-only? Perhaps we could apply 
write access for these special cases only?

Thanks,
Mike

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Nicolas Pitre]

On Tue, 12 Apr 2011, Michael Bohan wrote:

> Hi,
> 
> In arch/arm/kernel/traps.c:set_tls() and arch/arm/include/asm/tls.h, some
> configurations allow for a assignment of address 0xffff0ff0. This address
> falls within the MT_HIGH_VECTORS mapping setup in devicemaps_init(). That
> mapping is explicitly made read-only. Thus, the kernel takes a segfault when
> writing in set_tls().

If set_tls() writes to 0xffff0ff0 in your case, then you must have an 
ARM core which is prior ARMv6k.

> It looks like this disparity may have been introduced in this commit:
> 
> commit 36bb94ba36f332de767cfaa3af6a5136435a3a9c
> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date:   Tue Nov 16 08:40:36 2010 +0000
> 
>     ARM: pgtable: provide RDONLY page table bit rather than WRITE bit
> 
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Is there a reason this mapping must be read-only?

It is read-only for user space to prevent user space from messing with 
the vector table.

In the kernel, it is read-only _only_ when CONFIG_CPU_USE_DOMAINS is not 
enabled which may happen with ARMv6k and above.  Otherwise, if you are 
pre ARMv6k, you do use domains, and then the vector page is read-write 
for the kernel.

> Perhaps we could apply write
> access for these special cases only?

I'd rather suggest you investigate what changes you did to your kernel 
tree that would explain the apparent inconsistency in your kernel 
config.


Nicolas

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Michael Bohan]

On 4/12/2011 8:31 PM, Nicolas Pitre wrote:
> On Tue, 12 Apr 2011, Michael Bohan wrote:
> If set_tls() writes to 0xffff0ff0 in your case, then you must have an
> ARM core which is prior ARMv6k.

Actually, no, but for historical reasons we were not using the hwreg TLS 
support for all configurations. Our libc expected the data at this address.

>> Is there a reason this mapping must be read-only?
>
> It is read-only for user space to prevent user space from messing with
> the vector table.
>
> In the kernel, it is read-only _only_ when CONFIG_CPU_USE_DOMAINS is not
> enabled which may happen with ARMv6k and above.  Otherwise, if you are
> pre ARMv6k, you do use domains, and then the vector page is read-write
> for the kernel.

Yes, this seems to be the the key. In previous versions the kernel was 
given this privilege unconditionally in arch/arm/mm/proc-macros.S.

>> Perhaps we could apply write
>> access for these special cases only?
>
> I'd rather suggest you investigate what changes you did to your kernel
> tree that would explain the apparent inconsistency in your kernel
> config.

Yes, the inconsistency was on our end. We will revert all hacks and 
mandate that our libc do things correctly.

Thanks,
Mike

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Nicolas Pitre]

On Tue, 19 Apr 2011, Michael Bohan wrote:

> On 4/12/2011 8:31 PM, Nicolas Pitre wrote:
> > On Tue, 12 Apr 2011, Michael Bohan wrote:
> > If set_tls() writes to 0xffff0ff0 in your case, then you must have an
> > ARM core which is prior ARMv6k.
> 
> Actually, no, but for historical reasons we were not using the hwreg TLS
> support for all configurations. Our libc expected the data at this address.

Are you saying that your user space libc was reading at 0xffff0ff0 
directly?  I hope not, because if you did so, you clearly abused the 
interface and the contract between user space and the kernel.  Here's 
what I wrote in the comment right above the related code:

 * These are segment of kernel provided user code reachable from user space
 * at a fixed address in kernel memory.  This is used to provide user space
 * with some operations which require kernel help because of unimplemented
 * native feature and/or instructions in many ARM CPUs. The idea is for
 * this code to be executed directly in user mode for best efficiency but
 * which is too intimate with the kernel counter part to be left to user
 * libraries.  In fact this code might even differ from one CPU to another
 * depending on the available  instruction set and restrictions like on
 * SMP systems.  In other words, the kernel reserves the right to change
 * this code as needed without warning. Only the entry points and their
 * results are guaranteed to be stable.

This has been there since April 29th 2005 i.e. 6 years ago.

> > > Is there a reason this mapping must be read-only?
> > 
> > It is read-only for user space to prevent user space from messing with
> > the vector table.
> > 
> > In the kernel, it is read-only _only_ when CONFIG_CPU_USE_DOMAINS is not
> > enabled which may happen with ARMv6k and above.  Otherwise, if you are
> > pre ARMv6k, you do use domains, and then the vector page is read-write
> > for the kernel.
> 
> Yes, this seems to be the the key. In previous versions the kernel was given
> this privilege unconditionally in arch/arm/mm/proc-macros.S.
> 
> > > Perhaps we could apply write
> > > access for these special cases only?
> > 
> > I'd rather suggest you investigate what changes you did to your kernel
> > tree that would explain the apparent inconsistency in your kernel
> > config.
> 
> Yes, the inconsistency was on our end. We will revert all hacks and mandate
> that our libc do things correctly.

Good!  Search for "User helpers" in arch/arm/kernel/head-armv.S and see 
following comments if you want proper usage examples.


Nicolas

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Michael Bohan]

On 4/19/2011 5:21 PM, Nicolas Pitre wrote:
> Are you saying that your user space libc was reading at 0xffff0ff0
> directly?  I hope not, because if you did so, you clearly abused the
> interface and the contract between user space and the kernel.  Here's
> what I wrote in the comment right above the related code:
>
>   * These are segment of kernel provided user code reachable from user space
>   * at a fixed address in kernel memory.  This is used to provide user space
>   * with some operations which require kernel help because of unimplemented
>   * native feature and/or instructions in many ARM CPUs. The idea is for
>   * this code to be executed directly in user mode for best efficiency but
>   * which is too intimate with the kernel counter part to be left to user
>   * libraries.  In fact this code might even differ from one CPU to another
>   * depending on the available  instruction set and restrictions like on
>   * SMP systems.  In other words, the kernel reserves the right to change
>   * this code as needed without warning. Only the entry points and their
>   * results are guaranteed to be stable.
>
> This has been there since April 29th 2005 i.e. 6 years ago.

Yes, unfortunately Android appears to do this as an 'optimization' in 
the case of dynamically linked execs. That is, it skips the helper code 
all together.

Mike

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Nicolas Pitre]

On Tue, 19 Apr 2011, Michael Bohan wrote:

> On 4/19/2011 5:21 PM, Nicolas Pitre wrote:
> > Are you saying that your user space libc was reading at 0xffff0ff0
> > directly?  I hope not, because if you did so, you clearly abused the
> > interface and the contract between user space and the kernel.  Here's
> > what I wrote in the comment right above the related code:
> > 
> >   * These are segment of kernel provided user code reachable from user space
> >   * at a fixed address in kernel memory.  This is used to provide user space
> >   * with some operations which require kernel help because of unimplemented
> >   * native feature and/or instructions in many ARM CPUs. The idea is for
> >   * this code to be executed directly in user mode for best efficiency but
> >   * which is too intimate with the kernel counter part to be left to user
> >   * libraries.  In fact this code might even differ from one CPU to another
> >   * depending on the available  instruction set and restrictions like on
> >   * SMP systems.  In other words, the kernel reserves the right to change
> >   * this code as needed without warning. Only the entry points and their
> >   * results are guaranteed to be stable.
> > 
> > This has been there since April 29th 2005 i.e. 6 years ago.
> 
> Yes, unfortunately Android appears to do this as an 'optimization' in the case
> of dynamically linked execs. That is, it skips the helper code all together.

You should find the persons responsible for this aberration and flame 
them with extreme prejudice!  As if this could make any measurable 
difference on any benchmark...

Either you have the hw reg for TLS and may use it directly, or you use 
the provided helper dammit!  I think I'll just move the implementation 
private 0xffff0ff0 location around just to make sure it breaks any user 
code that wrongly started to abuse this interface.


Nicolas

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Colin Cross]

On Tue, Apr 19, 2011 at 8:01 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 19 Apr 2011, Michael Bohan wrote:
>
>> On 4/19/2011 5:21 PM, Nicolas Pitre wrote:
>> > Are you saying that your user space libc was reading at 0xffff0ff0
>> > directly? ?I hope not, because if you did so, you clearly abused the
>> > interface and the contract between user space and the kernel. ?Here's
>> > what I wrote in the comment right above the related code:
>> >
>> > ? * These are segment of kernel provided user code reachable from user space
>> > ? * at a fixed address in kernel memory. ?This is used to provide user space
>> > ? * with some operations which require kernel help because of unimplemented
>> > ? * native feature and/or instructions in many ARM CPUs. The idea is for
>> > ? * this code to be executed directly in user mode for best efficiency but
>> > ? * which is too intimate with the kernel counter part to be left to user
>> > ? * libraries. ?In fact this code might even differ from one CPU to another
>> > ? * depending on the available ?instruction set and restrictions like on
>> > ? * SMP systems. ?In other words, the kernel reserves the right to change
>> > ? * this code as needed without warning. Only the entry points and their
>> > ? * results are guaranteed to be stable.
>> >
>> > This has been there since April 29th 2005 i.e. 6 years ago.
>>
>> Yes, unfortunately Android appears to do this as an 'optimization' in the case
>> of dynamically linked execs. That is, it skips the helper code all together.
>
> You should find the persons responsible for this aberration and flame
> them with extreme prejudice! ?As if this could make any measurable
> difference on any benchmark...
>
> Either you have the hw reg for TLS and may use it directly, or you use
> the provided helper dammit! ?I think I'll just move the implementation
> private 0xffff0ff0 location around just to make sure it breaks any user
> code that wrongly started to abuse this interface.
>

I believe the original implementation used the helper, and then it was
modified to skip the helper due to a measured performance impact.
Something to do with cache pressure and a specific vendor GL library
that heavily used get_tls(), I think, but it was before my time.  It
is already fixed on the Android userspace side by defining
ARCH_ARM_HAVE_TLS_REGISTER, which will prevent libc from ever
accessing the private location, and we have dropped the hacks from our
recent kernel trees.

MT_HIGH_VECTOR mapping set read-only creating illegal access

[Russell King - ARM Linux]

On Tue, Apr 12, 2011 at 05:42:24PM -0700, Michael Bohan wrote:
> Hi,
>
> In arch/arm/kernel/traps.c:set_tls() and arch/arm/include/asm/tls.h,  
> some configurations allow for a assignment of address 0xffff0ff0. This  
> address falls within the MT_HIGH_VECTORS mapping setup in  
> devicemaps_init(). That mapping is explicitly made read-only. Thus, the  
> kernel takes a segfault when writing in set_tls().
>
> It looks like this disparity may have been introduced in this commit:
>
> commit 36bb94ba36f332de767cfaa3af6a5136435a3a9c
> Author: Russell King <rmk+kernel@arm.linux.org.uk>
> Date:   Tue Nov 16 08:40:36 2010 +0000
>
>     ARM: pgtable: provide RDONLY page table bit rather than WRITE bit
>
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>
> Is there a reason this mapping must be read-only? Perhaps we could apply  
> write access for these special cases only?

Have you checked the behaviour immediately before this commit, and the
behaviour with this commit applied?