MT_HIGH_VECTOR mapping set read-only creating illegal access
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 <[email protected]> Date: Tue Nov 16 08:40:36 2010 +0000
ARM: pgtable: provide RDONLY page table bit rather than WRITE bit
Is there a reason this mapping must be read-only? Perhaps we could apply write access for these special cases only?
Thanks,
Mike
Signature · 2 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Quoted 7 lines · 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 address0xffff0ff0. This address falls within theMT_HIGH_VECTORSmapping setup indevicemaps_init(). That mapping is explicitly made read-only. Thus, the kernel takes a segfault when writing inset_tls().
If set_tls() writes to 0xffff0ff0 in your case, then you must have an ARM core which is prior ARMv6k.
Quoted 11 lines
It looks like this disparity may have been introduced in this commit:
commit
36bb94ba36f332de767cfaa3af6a5136435a3a9cAuthor: Russell King <[email protected]> Date: Tue Nov 16 08:40:36 2010 +0000ARM: pgtable: provide RDONLY page table bit rather than WRITE bit
Signed-off-by Russell King <[email protected]>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.
Quoted 2 lines
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
Quoted 3 lines · Nicolas Pitre
On Tue, 12 Apr 2011, Michael Bohan wrote:
Ifset_tls()writes to0xffff0ff0in 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.
Quoted 9 lines
Quoted 1 line
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_DOMAINSis 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.
Quoted 6 lines
Quoted 2 lines
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
Signature · 2 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Quoted 7 lines · Michael Bohan
Quoted 3 lines · Nicolas Pitre
On Tue, 12 Apr 2011, Michael Bohan wrote:
Ifset_tls()writes to0xffff0ff0in 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.
Quoted 16 lines
Quoted 9 lines
Quoted 1 line
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_DOMAINSis 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.Quoted 3 lines
Quoted 2 lines
Perhaps we could apply write
access for these special cases only?
Quoted 6 lines
Quoted 3 lines
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
Quoted 17 lines · Nicolas Pitre
Are you saying that your user space libc was reading at
0xffff0ff0directly? 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.
Quoted 1 line
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
Signature · 2 lines
Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Quoted 18 lines · Michael Bohan
Quoted 17 lines · Nicolas Pitre
Are you saying that your user space libc was reading at
0xffff0ff0directly? 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.
Quoted 4 lines
Quoted 1 line
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
Quoted 25 lines · Nicolas Pitre
Quoted 18 lines · Michael Bohan
Quoted 17 lines · Nicolas Pitre
Are you saying that your user space libc was reading at
0xffff0ff0directly? ?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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 9 lines
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
0xffff0ff0location 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.
Quoted 14 lines · 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 address0xffff0ff0. This address falls within theMT_HIGH_VECTORSmapping setup indevicemaps_init(). That mapping is explicitly made read-only. Thus, the kernel takes a segfault when writing inset_tls().It looks like this disparity may have been introduced in this commit:
commit
36bb94ba36f332de767cfaa3af6a5136435a3a9cAuthor: Russell King <[email protected]> Date: Tue Nov 16 08:40:36 2010 +0000
Quoted 6 lines
ARM: pgtable: provide RDONLY page table bit rather than WRITE bit
Signed-off-by Russell King <[email protected]>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?