[Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@domain.hid>

Page protection changes issued via mprotect, e.g. to enable executable
stacks, cause spurious minor faults as they remove the write permission
from the modified range again. Avoid this by faking shared pages so that
vm_get_page_prot returns the desired page permissions.

Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
---
 mm/mprotect.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/mm/mprotect.c b/mm/mprotect.c
index 5a688a2..3f234b3 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -147,6 +147,7 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev,
 {
 	struct mm_struct *mm = vma->vm_mm;
 	unsigned long oldflags = vma->vm_flags;
+	unsigned long protflags;
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
 	pgoff_t pgoff;
@@ -205,8 +206,17 @@ success:
 	 * held in write mode.
 	 */
 	vma->vm_flags = newflags;
+	protflags = newflags;
+#ifdef CONFIG_IPIPE
+	/*
+	 * Enforce non-COW vm_page_prot by faking VM_SHARED on locked regions.
+	 */
+	if (test_bit(MMF_VM_PINNED, &mm->flags) &&
+	    ((vma->vm_flags | mm->def_flags) & VM_LOCKED))
+		protflags |= VM_SHARED;
+#endif
 	vma->vm_page_prot = pgprot_modify(vma->vm_page_prot,
-					  vm_get_page_prot(newflags));
+					  vm_get_page_prot(protflags));
 
 	if (vma_wants_writenotify(vma)) {
 		vma->vm_page_prot = vm_get_page_prot(newflags & ~VM_SHARED);
-- 
1.7.1

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Gilles Chanteperdrix]

On 07/07/2011 09:14 PM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@domain.hid>
> 
> Page protection changes issued via mprotect, e.g. to enable executable
> stacks, cause spurious minor faults as they remove the write permission
> from the modified range again. Avoid this by faking shared pages so that
> vm_get_page_prot returns the desired page permissions.

This looks dangerous to me. Have you checked that write to private heaps
will not end up corrupting shared data?

-- 
					    Gilles.

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>
>> Page protection changes issued via mprotect, e.g. to enable executable
>> stacks, cause spurious minor faults as they remove the write permission
>> from the modified range again. Avoid this by faking shared pages so that
>> vm_get_page_prot returns the desired page permissions.
> 
> This looks dangerous to me. Have you checked that write to private heaps
> will not end up corrupting shared data?

Can't follow this yet.

If you check the comment on protection_map in mm/mmap.c, the difference
between private and shared is in real write vs. COW-able write. That's
what my patch is exploiting to get the proper arch-dependent page
protection bits.

Are you aware of side effects or do you know a better way to inject
write permissions into the protection flags?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Gilles Chanteperdrix]

On 07/08/2011 02:15 PM, Jan Kiszka wrote:
> On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
>> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>> Page protection changes issued via mprotect, e.g. to enable executable
>>> stacks, cause spurious minor faults as they remove the write permission
>>> from the modified range again. Avoid this by faking shared pages so that
>>> vm_get_page_prot returns the desired page permissions.
>>
>> This looks dangerous to me. Have you checked that write to private heaps
>> will not end up corrupting shared data?
> 
> Can't follow this yet.
> 
> If you check the comment on protection_map in mm/mmap.c, the difference
> between private and shared is in real write vs. COW-able write. That's
> what my patch is exploiting to get the proper arch-dependent page
> protection bits.
> 
> Are you aware of side effects or do you know a better way to inject
> write permissions into the protection flags?

I would tend to think that shared and private mappings do not react the
same way when we write to them.

In order to avoid the fault on first access, I would trigger a fault for
each page of the mapping, the way we do it in __rthal_arm_fault_range
(in ksrc/arch/arm/hal.c), but in the I-pipe kernel.

-- 
                                                                Gilles.

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

On 2011-07-12 08:48, Gilles Chanteperdrix wrote:
> On 07/08/2011 02:15 PM, Jan Kiszka wrote:
>> On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
>>> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>> Page protection changes issued via mprotect, e.g. to enable executable
>>>> stacks, cause spurious minor faults as they remove the write permission
>>>> from the modified range again. Avoid this by faking shared pages so that
>>>> vm_get_page_prot returns the desired page permissions.
>>>
>>> This looks dangerous to me. Have you checked that write to private heaps
>>> will not end up corrupting shared data?
>>
>> Can't follow this yet.
>>
>> If you check the comment on protection_map in mm/mmap.c, the difference
>> between private and shared is in real write vs. COW-able write. That's
>> what my patch is exploiting to get the proper arch-dependent page
>> protection bits.
>>
>> Are you aware of side effects or do you know a better way to inject
>> write permissions into the protection flags?
> 
> I would tend to think that shared and private mappings do not react the
> same way when we write to them.

Yes, private mappings can undergo COW.

> 
> In order to avoid the fault on first access, I would trigger a fault for
> each page of the mapping, the way we do it in __rthal_arm_fault_range
> (in ksrc/arch/arm/hal.c), but in the I-pipe kernel.

That sounds like overkill, and it's potentially racy (mprotect may open
a short window where the active PTEs do not allow writes for
concurrently running threads).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Gilles Chanteperdrix]

On 07/12/2011 09:15 AM, Jan Kiszka wrote:
> On 2011-07-12 08:48, Gilles Chanteperdrix wrote:
>> On 07/08/2011 02:15 PM, Jan Kiszka wrote:
>>> On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
>>>> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>>> Page protection changes issued via mprotect, e.g. to enable executable
>>>>> stacks, cause spurious minor faults as they remove the write permission
>>>>> from the modified range again. Avoid this by faking shared pages so that
>>>>> vm_get_page_prot returns the desired page permissions.
>>>>
>>>> This looks dangerous to me. Have you checked that write to private heaps
>>>> will not end up corrupting shared data?
>>>
>>> Can't follow this yet.
>>>
>>> If you check the comment on protection_map in mm/mmap.c, the difference
>>> between private and shared is in real write vs. COW-able write. That's
>>> what my patch is exploiting to get the proper arch-dependent page
>>> protection bits.
>>>
>>> Are you aware of side effects or do you know a better way to inject
>>> write permissions into the protection flags?
>>
>> I would tend to think that shared and private mappings do not react the
>> same way when we write to them.
> 
> Yes, private mappings can undergo COW.

My point exactly, so, say, if the mapping is an anonymous mapping with
only the zero page mapped, you will allow writing to the zero page, this
does not look like a sane thing to do. The same goes for private file
mappings where the code need to be modified (for instance relocations
when mixing non PIC code with PIC code).

> 
>>
>> In order to avoid the fault on first access, I would trigger a fault for
>> each page of the mapping, the way we do it in __rthal_arm_fault_range
>> (in ksrc/arch/arm/hal.c), but in the I-pipe kernel.
> 
> That sounds like overkill, and it's potentially racy (mprotect may open
> a short window where the active PTEs do not allow writes for
> concurrently running threads).

If a program starts writing to a mapping before mprotect returns, it is
already bogus anyway (I am talking about triggering the fault in
mprotect code of course).


> 
> Jan
> 


-- 
                                                                Gilles.

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Jan Kiszka]

On 2011-07-27 20:41, Gilles Chanteperdrix wrote:
> On 07/12/2011 09:15 AM, Jan Kiszka wrote:
>> On 2011-07-12 08:48, Gilles Chanteperdrix wrote:
>>> On 07/08/2011 02:15 PM, Jan Kiszka wrote:
>>>> On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
>>>>> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>
>>>>>> Page protection changes issued via mprotect, e.g. to enable executable
>>>>>> stacks, cause spurious minor faults as they remove the write permission
>>>>>> from the modified range again. Avoid this by faking shared pages so that
>>>>>> vm_get_page_prot returns the desired page permissions.
>>>>>
>>>>> This looks dangerous to me. Have you checked that write to private heaps
>>>>> will not end up corrupting shared data?
>>>>
>>>> Can't follow this yet.
>>>>
>>>> If you check the comment on protection_map in mm/mmap.c, the difference
>>>> between private and shared is in real write vs. COW-able write. That's
>>>> what my patch is exploiting to get the proper arch-dependent page
>>>> protection bits.
>>>>
>>>> Are you aware of side effects or do you know a better way to inject
>>>> write permissions into the protection flags?
>>>
>>> I would tend to think that shared and private mappings do not react the
>>> same way when we write to them.
>>
>> Yes, private mappings can undergo COW.
> 
> My point exactly, so, say, if the mapping is an anonymous mapping with
> only the zero page mapped, you will allow writing to the zero page, this
> does not look like a sane thing to do. The same goes for private file
> mappings where the code need to be modified (for instance relocations
> when mixing non PIC code with PIC code).

To my understanding, and testing seems to confirm this, the branch under
vma_wants_writenotify in mprotect_fixup: if there is a need to fault
even on shared pages (or pseudo-shared in our case), vm_page_prot will
be overwritten anyway.

> 
>>
>>>
>>> In order to avoid the fault on first access, I would trigger a fault for
>>> each page of the mapping, the way we do it in __rthal_arm_fault_range
>>> (in ksrc/arch/arm/hal.c), but in the I-pipe kernel.
>>
>> That sounds like overkill, and it's potentially racy (mprotect may open
>> a short window where the active PTEs do not allow writes for
>> concurrently running threads).
> 
> If a program starts writing to a mapping before mprotect returns, it is
> already bogus anyway (I am talking about triggering the fault in
> mprotect code of course).

Unfortunately, there is also the case where the page was writable before
mprotect and should remains so afterward, e.g. if you flip the execution
right. So this race is real.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Adeos-main] [PATCH] ipipe: Prevent unwritable pages after mprotect

[Jan Kiszka]

On 2011-07-28 18:26, Jan Kiszka wrote:
> On 2011-07-27 20:41, Gilles Chanteperdrix wrote:
>> On 07/12/2011 09:15 AM, Jan Kiszka wrote:
>>> On 2011-07-12 08:48, Gilles Chanteperdrix wrote:
>>>> On 07/08/2011 02:15 PM, Jan Kiszka wrote:
>>>>> On 2011-07-08 13:55, Gilles Chanteperdrix wrote:
>>>>>> On 07/07/2011 09:14 PM, Jan Kiszka wrote:
>>>>>>> From: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>
>>>>>>> Page protection changes issued via mprotect, e.g. to enable executable
>>>>>>> stacks, cause spurious minor faults as they remove the write permission
>>>>>>> from the modified range again. Avoid this by faking shared pages so that
>>>>>>> vm_get_page_prot returns the desired page permissions.
>>>>>>
>>>>>> This looks dangerous to me. Have you checked that write to private heaps
>>>>>> will not end up corrupting shared data?
>>>>>
>>>>> Can't follow this yet.
>>>>>
>>>>> If you check the comment on protection_map in mm/mmap.c, the difference
>>>>> between private and shared is in real write vs. COW-able write. That's
>>>>> what my patch is exploiting to get the proper arch-dependent page
>>>>> protection bits.
>>>>>
>>>>> Are you aware of side effects or do you know a better way to inject
>>>>> write permissions into the protection flags?
>>>>
>>>> I would tend to think that shared and private mappings do not react the
>>>> same way when we write to them.
>>>
>>> Yes, private mappings can undergo COW.
>>
>> My point exactly, so, say, if the mapping is an anonymous mapping with
>> only the zero page mapped, you will allow writing to the zero page, this
>> does not look like a sane thing to do. The same goes for private file
>> mappings where the code need to be modified (for instance relocations
>> when mixing non PIC code with PIC code).
> 
> To my understanding, and testing seems to confirm this, the branch under
> vma_wants_writenotify in mprotect_fixup: if there is a need to fault
                                         ^handles this

> even on shared pages (or pseudo-shared in our case), vm_page_prot will
> be overwritten anyway.
> 

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux