ARM: uprobes xol write directly to userspace

kernel/events/uprobes.c +5 -0

+void __weak arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
+				  void *src, unsigned long len)
+{
+	/*
+	 * Note if CPU does not support instructions write snooping

kernel/events/uprobes.c static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) · line 1152

Quoted 9 lines

kernel/events/uprobes.c +1 -1

@@ -1149,7 +1149,7 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	}
 
 	ret = install_special_mapping(mm, area->vaddr, PAGE_SIZE,
-				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, &area->page);
+				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_WRITE, &area->page);

Yes, this is nasty.

I would like to have a reason to nack this change ;) Unfortunately the current code is buggy too and we need to protect the kernel from malicious applications which can rewrite the insn we are going to step over in UTASK_SSTEP state anyway.

From: Oleg Nesterov <[email protected]>
Date: Wed, 16 Apr 2014 16:51:07 +0200

Quoted 13 lines · Victor Kamensky

kernel/events/uprobes.c static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) · line 1152

Quoted 9 lines

kernel/events/uprobes.c +1 -1

@@ -1149,7 +1149,7 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	}
 
 	ret = install_special_mapping(mm, area->vaddr, PAGE_SIZE,
-				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO, &area->page);
+				VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO|VM_WRITE, &area->page);

Yes, this is nasty.

Quoted 3 lines

I would like to have a reason to nack this change ;) Unfortunately the current code is buggy too and we need to protect the kernel from malicious applications which can rewrite the insn we are going to step over in UTASK_SSTEP state anyway.

I think there may be a way to achieve your objectives.

Pass MAP_SHARED into the flags argument of get_unmapped_area(), and pass the pfn of the xol page in as "pgoff".

This will make the xol page get mapped into the user process at an address which is "D-cache congruent" to the kernel side mapping.

So all kernel stores to the page will use the same D-cache line that user space accesses to it will.

So we end up with all of the benefits of storing directly to userspace, along with what you're trying to achieve.

But did you really mean get_unmapped_area(pgoff => page_to_pfn(area->page)) ?

I simply can't understand how this can work, arm (and x86) really use it as "pgoff << PAGE_SHIFT" align_offset accounted in unmapped_area() ?

Quoted 2 lines

So we end up with all of the benefits of storing directly to userspace, along with what you're trying to achieve.

And in this case we can avoid copy_to_user(), right ?

From: Oleg Nesterov <[email protected]>
Date: Wed, 16 Apr 2014 18:43:10 +0200

Quoted 1 line

But did you really mean get_unmapped_area(pgoff => page_to_pfn(area->page)) ?

Yes.

Quoted 2 lines

I simply can't understand how this can work, arm (and x86) really use it as "pgoff << PAGE_SHIFT" align_offset accounted in unmapped_area() ?

When a platform has D-cache aliasing issues, it must make sure that every shared page is mapped to the same D-cache alias in all such shared mappings.

The way this is done is to map each pgoff to a page in the D-cache. For example, pgoff 0 would be given a virtual address that maps to the first page in the D-cache, pgoff 1 to the second, and so forth.

What we're doing with this get_unmapped_area() call is to make it so that userspace's virtual address will land at the same virtual alias as the kernel one does.

Quoted 4 lines

Quoted 2 lines

So we end up with all of the benefits of storing directly to userspace, along with what you're trying to achieve.

And in this case we can avoid copy_to_user(), right ?

Yes, that's the whole idea, you can forget about the VM_WRITE etc. that caused your concerns.

It'd be just memcpy to kernel side mapping of the page + i-cache flush where necessary.

The last question... area->page = alloc_page(GFP_HIGHUSER), and I am not sure that arch/arm/mm/highmem.c:kmap_atomic() can't break the aliasing, __fix_to_virt() in this case will use the same (per-cpu) idx.

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

From: Oleg Nesterov <[email protected]>
Date: Wed, 16 Apr 2014 21:18:25 +0200

Quoted 6 lines

The last question... area->page = alloc_page(GFP_HIGHUSER), and I am not sure that arch/arm/mm/highmem.c:kmap_atomic() can't break the aliasing, __fix_to_virt() in this case will use the same (per-cpu) idx.

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

Good point, it might therefore make sense to use a low-mem page.

Quoted 10 lines

From: Oleg Nesterov <[email protected]>
Date: Wed, 16 Apr 2014 21:18:25 +0200

Quoted 6 lines

The last question... area->page = alloc_page(GFP_HIGHUSER), and I am not sure that arch/arm/mm/highmem.c:kmap_atomic() can't break the aliasing, __fix_to_virt() in this case will use the same (per-cpu) idx.

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

Quoted 2 lines

Good point, it might therefore make sense to use a low-mem page.

The following test code seems to have the same problems with stale user icache. It works if I put the dcache flush back in. Am I missing something?

From: David Long <[email protected]>
Date: Wed, 16 Apr 2014 16:24:18 -0400

Quoted 14 lines · David Miller

Quoted 12 lines

From: Oleg Nesterov <[email protected]>
Date: Wed, 16 Apr 2014 21:18:25 +0200

Quoted 6 lines

The last question... area->page = alloc_page(GFP_HIGHUSER), and I am not sure that arch/arm/mm/highmem.c:kmap_atomic() can't break the aliasing, __fix_to_virt() in this case will use the same (per-cpu) idx.

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

Good point, it might therefore make sense to use a low-mem page.

Quoted 3 lines

The following test code seems to have the same problems with stale user icache. It works if I put the dcache flush back in. Am I missing something?

Weird, if we store to the kernel side it should be just a matter of clearing the I-cache out. There should be no D-cache aliasing whatsoever.

Maybe you could print out area->vaddr and
page_to_virt(area->page) so we can see if area->vaddr is choosen correctly?

Although I notice that flush_cache_user_range() on ARM flushes both D and I caches. And I think that's what userspace ends up triggering when it uses the system call that exists to support self-modifying and JIT code generators.

An ARM expert will have to chime in...

Weird, if we store to the kernel side it should be just a matter of clearing the I-cache out. There should be no D-cache aliasing whatsoever. Maybe you could print out area->vaddr and page_to_virt(area->page) so we can see if area->vaddr is choosen correctly?

Although I notice that flush_cache_user_range() on ARM flushes both D and I caches. And I think that's what userspace ends up triggering when it uses the system call that exists to support self-modifying and JIT code generators.

An ARM expert will have to chime in...

Quoted 11 lines · David Miller

Weird, if we store to the kernel side it should be just a matter of clearing the I-cache out. There should be no D-cache aliasing whatsoever. Maybe you could print out area->vaddr and page_to_virt(area->page) so we can see if area->vaddr is choosen correctly?

Although I notice that flush_cache_user_range() on ARM flushes both D and I caches. And I think that's what userspace ends up triggering when it uses the system call that exists to support self-modifying and JIT code generators.

Quoted 1 line

An ARM expert will have to chime in...

So, David's patch is against the existing kernel (I checked the blob ID in the patch.)

It looks like David just replaced flush_dcache_page() with flush_icache_all() as a hack. So my question is... between flush_dcache_page() and flush_icache_all(), what was the intermediary (if any) that was attempted?

Now, I'm going to fill in some details for DaveM. The type of the I/D L1 caches found on any particular architecture version of CPU can be:

Arch D-cache I-cache
ARMv7 VIPT nonaliasing VIVT ASID tagged
PIPT
-------------------------------------------------
ARMv6 VIPT nonalising VIPT nonaliasing
VIPT aliasing VIPT aliasing
-------------------------------------------------
ARMv5 VIVT VIVT
&older

(For ARMv6, each can be either/or, though practically, there's no point to I-aliasing and D-nonaliasing since it implies the I-cache is bigger than the D-cache.)

Our I-caches don't snoop/see the D-cache at all - so writes need to be pushed out to what we call the "point of unification" where the I and D streams meet. For anything we care about, that's normally the L2 cache - L1 cache is harvard, L2 cache is unified.

Hence, we don't care which D-alias (if any) the data is written, so long as it's pushed out of the L1 data cache so that it's visible to the L1 instruction cache.

If we're writing via a different mapping to that which is being executed, I think the safest thing to do is to flush it out of the L1 D-cache at the address it was written, and then flush any stale line from the L1 I-cache using the user address. This is quite a unique requirement, and we don't have anything which covers it. The closest you could get is to that using existing calls is:

1. write the new instruction
2. flush_dcache_page()
3. flush_cache_user_range() using the user address

and I think that should be safe on all the above cache types.

Our I-caches don't snoop/see the D-cache at all - so writes need to be pushed out to what we call the "point of unification" where the I and D streams meet. For anything we care about, that's normally the L2 cache - L1 cache is harvard, L2 cache is unified.

Hence, we don't care which D-alias (if any) the data is written, so long as it's pushed out of the L1 data cache so that it's visible to the L1 instruction cache.

If we're writing via a different mapping to that which is being executed, I think the safest thing to do is to flush it out of the L1 D-cache at the address it was written, and then flush any stale line from the L1 I-cache using the user address. This is quite a unique requirement, and we don't have anything which covers it. The closest you could get is to that using existing calls is:

1. write the new instruction
2. flush_dcache_page()
3. flush_cache_user_range() using the user address

and I think that should be safe on all the above cache types.

void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
const void *src, int len)

{
        void *kaddr = kmap_atomic(page);

#ifdef CONFIG_SMP
        preempt_disable();

#endif

Quoted 16 lines · Russell King - ARM Linux

Our I-caches don't snoop/see the D-cache at all - so writes need to be pushed out to what we call the "point of unification" where the I and D streams meet. For anything we care about, that's normally the L2 cache - L1 cache is harvard, L2 cache is unified.

Hence, we don't care which D-alias (if any) the data is written, so long as it's pushed out of the L1 data cache so that it's visible to the L1 instruction cache.

If we're writing via a different mapping to that which is being executed, I think the safest thing to do is to flush it out of the L1 D-cache at the address it was written, and then flush any stale line from the L1 I-cache using the user address. This is quite a unique requirement, and we don't have anything which covers it. The closest you could get is to that using existing calls is:

Quoted 6 lines

1. write the new instruction
2. flush_dcache_page()
3. flush_cache_user_range() using the user address

and I think that should be safe on all the above cache types.

It doesn't feel to me like we yet have a clear consensus on the appropriate near or long-term fix for this problem. I'm worried time is short to get a fix in for v3.15. I'm not sure how elegant that fix needs to be. I've gotten good test runs using a modified/simplified version of Victor's arch callback and a slight variation of Russell's sequence of operations from above:

void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
const void *src, int len)

{
        void *kaddr = kmap_atomic(page);

#ifdef CONFIG_SMP
        preempt_disable();
#endif
        memcpy(kaddr + (vaddr & ~PAGE_MASK), src, len);
        clean_dcache_area(kaddr, len);
        flush_cache_user_range(vaddr, vaddr + len);

#ifdef CONFIG_SMP
        preempt_enable();
#endif
        kunmap_atomic(kaddr);
}

I have to say using clean_dcache_area() to write back the two words that changed (and rest of the cache line of course) seems more appropriate than flushing a whole page. Are there implications in doing that which makes this a bad idea though?

At any rate, for v3.15 do we want to persue the more complex solutions with "congruent" mappings and use of copy_to_user(), or just something like the above (plus the rest of Victors v3 patch)? I'm sure Oleg is even less happy than me about yet another arch_ callback but we can hold out the hope that a more elegant solution can follow in the next release. One that might introduce risk we can't accept in v3.15 right now (e.g.: mapping the xol area writeable for all architectures).

I have also tested (somewhat) both Victor's unmodified v3 and v4 patches on exynos 5250 and found them to work.

Thanks,
-dl

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

Quoted 2 lines · Oleg Nesterov

Looks like, __kunmap_atomic()->__cpuc_flush_dcache_area() should take care, but could you please ack/nack my understanding?

flush_dcache_area() doesn't touch the I-cache... the hint is in the name. :) This is also the function which is used for flush_dcache_page() which we've already established isn't sufficient (for the same reason.)

Plus... we still would need to know the user address(es) to flush for the I-cache side...