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

Groups > linux.kernel > #1307850 > unrolled thread

[PATCH] arm64: Allow vmalloc regions to be set with set_memory_*

Back to article view | Back to linux.kernel

Contents

  [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Laura Abbott <labbott@fedoraproject.org> - 2016-01-12 22:50 +0100
    Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Alexei Starovoitov <alexei.starovoitov@gmail.com> - 2016-01-13 01:10 +0100
      Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Daniel Borkmann <daniel@iogearbox.net> - 2016-01-13 01:40 +0100
    Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-01-13 15:10 +0100
      Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-01-13 17:20 +0100
        Re: [PATCH] arm64: Allow vmalloc regions to be set with set_memory_* Laura Abbott <labbott@redhat.com> - 2016-01-15 00:10 +0100

#1307850 — [PATCH] arm64: Allow vmalloc regions to be set with set_memory_*

The range of set_memory_* is currently restricted to the module address
range because of difficulties in breaking down larger block sizes.
vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
function ranges and add a comment explaining why the range is restricted
the way it is.

Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
---
This should let the protections for the eBPF work as expected, I don't
know if there is some sort of self test for thatL.
---
 arch/arm64/mm/pageattr.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 3571c73..274208e 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -36,6 +36,26 @@ static int change_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
 	return 0;
 }
 
+static bool validate_addr(unsigned long start, unsigned long end)
+{
+	/*
+	 * This check explicitly excludes most kernel memory. Most kernel
+	 * memory is mapped with a larger page size and breaking down the
+	 * larger page size without causing TLB conflicts is very difficult.
+	 *
+	 * If you need to call set_memory_* on a range, the recommendation is
+	 * to use vmalloc since that range is mapped with pages.
+	 */
+	if (start >= MODULES_VADDR && start < MODULES_END &&
+	    end >= MODULES_VADDR && end < MODULES_END)
+		return true;
+
+	if (is_vmalloc_addr(start) && is_vmalloc_addr(end))
+		return true;
+
+	return false;
+}
+
 static int change_memory_common(unsigned long addr, int numpages,
 				pgprot_t set_mask, pgprot_t clear_mask)
 {
@@ -51,10 +71,7 @@ static int change_memory_common(unsigned long addr, int numpages,
 		WARN_ON_ONCE(1);
 	}
 
-	if (start < MODULES_VADDR || start >= MODULES_END)
-		return -EINVAL;
-
-	if (end < MODULES_VADDR || end >= MODULES_END)
+	if (!validate_addr(start, end))
 		return -EINVAL;
 
 	data.set_mask = set_mask;
-- 
2.5.0

[toc] | [next] | [standalone]

#1307929

On Tue, Jan 12, 2016 at 01:46:27PM -0800, Laura Abbott wrote:
> 
> The range of set_memory_* is currently restricted to the module address
> range because of difficulties in breaking down larger block sizes.
> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
> function ranges and add a comment explaining why the range is restricted
> the way it is.
> 
> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
> ---
> This should let the protections for the eBPF work as expected, I don't
> know if there is some sort of self test for thatL.

you can test it with:
# sysctl net.core.bpf_jit_enable=1
# insmod test_bpf.ko

On x64 it shows:
test_bpf: Summary: 291 PASSED, 0 FAILED, [282/283 JIT'ed]

arm64 may have less JIT'ed tests.

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

#1307940

On 01/13/2016 01:01 AM, Alexei Starovoitov wrote:
> On Tue, Jan 12, 2016 at 01:46:27PM -0800, Laura Abbott wrote:
>>
>> The range of set_memory_* is currently restricted to the module address
>> range because of difficulties in breaking down larger block sizes.
>> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
>> function ranges and add a comment explaining why the range is restricted
>> the way it is.
>>
>> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
>> ---
>> This should let the protections for the eBPF work as expected, I don't
>> know if there is some sort of self test for thatL.
>
> you can test it with:
> # sysctl net.core.bpf_jit_enable=1
> # insmod test_bpf.ko
>
> On x64 it shows:
> test_bpf: Summary: 291 PASSED, 0 FAILED, [282/283 JIT'ed]
>
> arm64 may have less JIT'ed tests.

Also, in that lib/test_bpf.c file, you can do a test by overwriting/'corrupting'
part of the fp->insnsi instructions right after bpf_prog_select_runtime(fp) to
see if setting the bpf_prog as RO works.

Thanks,
Daniel

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

#1308438

On 12 January 2016 at 22:46, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> The range of set_memory_* is currently restricted to the module address
> range because of difficulties in breaking down larger block sizes.
> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
> function ranges and add a comment explaining why the range is restricted
> the way it is.
>
> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
> ---
> This should let the protections for the eBPF work as expected, I don't
> know if there is some sort of self test for thatL.


This is going to conflict with my KASLR implementation, since it puts
the kernel image right in the middle of the vmalloc area, and the
kernel is obviously mapped with block mappings. In fact, I am
proposing enabling huge-vmap for arm64 as well, since it seems an
improvement generally, but also specifically allows me to unmap the
__init section using the generic vunmap code (remove_vm_area). But in
general, I think the assumption that the whole vmalloc area is mapped
using pages is not tenable.

AFAICT, vmalloc still use pages exclusively even with huge-vmap (but
ioremap does not). So perhaps it would make sense to check for the
VM_ALLOC bit in the VMA flags (which I will not set for the kernel
regions either)


> ---
>  arch/arm64/mm/pageattr.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 3571c73..274208e 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -36,6 +36,26 @@ static int change_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
>         return 0;
>  }
>
> +static bool validate_addr(unsigned long start, unsigned long end)
> +{
> +       /*
> +        * This check explicitly excludes most kernel memory. Most kernel
> +        * memory is mapped with a larger page size and breaking down the
> +        * larger page size without causing TLB conflicts is very difficult.
> +        *
> +        * If you need to call set_memory_* on a range, the recommendation is
> +        * to use vmalloc since that range is mapped with pages.
> +        */
> +       if (start >= MODULES_VADDR && start < MODULES_END &&
> +           end >= MODULES_VADDR && end < MODULES_END)
> +               return true;
> +
> +       if (is_vmalloc_addr(start) && is_vmalloc_addr(end))
> +               return true;
> +
> +       return false;
> +}
> +
>  static int change_memory_common(unsigned long addr, int numpages,
>                                 pgprot_t set_mask, pgprot_t clear_mask)
>  {
> @@ -51,10 +71,7 @@ static int change_memory_common(unsigned long addr, int numpages,
>                 WARN_ON_ONCE(1);
>         }
>
> -       if (start < MODULES_VADDR || start >= MODULES_END)
> -               return -EINVAL;
> -
> -       if (end < MODULES_VADDR || end >= MODULES_END)
> +       if (!validate_addr(start, end))
>                 return -EINVAL;
>
>         data.set_mask = set_mask;
> --
> 2.5.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

#1308558

On 13 January 2016 at 15:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 January 2016 at 22:46, Laura Abbott <labbott@fedoraproject.org> wrote:
>>
>> The range of set_memory_* is currently restricted to the module address
>> range because of difficulties in breaking down larger block sizes.
>> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
>> function ranges and add a comment explaining why the range is restricted
>> the way it is.
>>
>> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
>> ---
>> This should let the protections for the eBPF work as expected, I don't
>> know if there is some sort of self test for thatL.
>
>
> This is going to conflict with my KASLR implementation, since it puts
> the kernel image right in the middle of the vmalloc area, and the
> kernel is obviously mapped with block mappings. In fact, I am
> proposing enabling huge-vmap for arm64 as well, since it seems an
> improvement generally, but also specifically allows me to unmap the
> __init section using the generic vunmap code (remove_vm_area). But in
> general, I think the assumption that the whole vmalloc area is mapped
> using pages is not tenable.
>
> AFAICT, vmalloc still use pages exclusively even with huge-vmap (but
> ioremap does not). So perhaps it would make sense to check for the
> VM_ALLOC bit in the VMA flags (which I will not set for the kernel
> regions either)
>

Something along these lines, perhaps?

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 3571c7309c5e..bda0a776c58e 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/vmalloc.h>
 #include <linux/sched.h>

 #include <asm/pgtable.h>
@@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr
        unsigned long end = start + size;
        int ret;
        struct page_change_data data;
+       struct vm_struct *area;

        if (!PAGE_ALIGNED(addr)) {
                start &= PAGE_MASK;
@@ -51,10 +53,14 @@ static int change_memory_common(unsigned long addr,
                WARN_ON_ONCE(1);
        }

-       if (start < MODULES_VADDR || start >= MODULES_END)
-               return -EINVAL;
-
-       if (end < MODULES_VADDR || end >= MODULES_END)
+       /*
+        * Check whether the [addr, addr + size) interval is entirely
+        * covered by precisely one VM area that has the VM_ALLOC flag set
+        */
+       area = find_vm_area((void *)addr);
+       if (!area ||
+           end > (unsigned long)area->addr + area->size ||
+           !(area->flags & VM_ALLOC))
                return -EINVAL;

        data.set_mask = set_mask;


>
>> ---
>>  arch/arm64/mm/pageattr.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>> index 3571c73..274208e 100644
>> --- a/arch/arm64/mm/pageattr.c
>> +++ b/arch/arm64/mm/pageattr.c
>> @@ -36,6 +36,26 @@ static int change_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
>>         return 0;
>>  }
>>
>> +static bool validate_addr(unsigned long start, unsigned long end)
>> +{
>> +       /*
>> +        * This check explicitly excludes most kernel memory. Most kernel
>> +        * memory is mapped with a larger page size and breaking down the
>> +        * larger page size without causing TLB conflicts is very difficult.
>> +        *
>> +        * If you need to call set_memory_* on a range, the recommendation is
>> +        * to use vmalloc since that range is mapped with pages.
>> +        */
>> +       if (start >= MODULES_VADDR && start < MODULES_END &&
>> +           end >= MODULES_VADDR && end < MODULES_END)
>> +               return true;
>> +
>> +       if (is_vmalloc_addr(start) && is_vmalloc_addr(end))
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>>  static int change_memory_common(unsigned long addr, int numpages,
>>                                 pgprot_t set_mask, pgprot_t clear_mask)
>>  {
>> @@ -51,10 +71,7 @@ static int change_memory_common(unsigned long addr, int numpages,
>>                 WARN_ON_ONCE(1);
>>         }
>>
>> -       if (start < MODULES_VADDR || start >= MODULES_END)
>> -               return -EINVAL;
>> -
>> -       if (end < MODULES_VADDR || end >= MODULES_END)
>> +       if (!validate_addr(start, end))
>>                 return -EINVAL;
>>
>>         data.set_mask = set_mask;
>> --
>> 2.5.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

#1309751

On 01/13/2016 08:10 AM, Ard Biesheuvel wrote:
> On 13 January 2016 at 15:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 12 January 2016 at 22:46, Laura Abbott <labbott@fedoraproject.org> wrote:
>>>
>>> The range of set_memory_* is currently restricted to the module address
>>> range because of difficulties in breaking down larger block sizes.
>>> vmalloc maps PAGE_SIZE pages so it is safe to use as well. Update the
>>> function ranges and add a comment explaining why the range is restricted
>>> the way it is.
>>>
>>> Signed-off-by: Laura Abbott <labbott@fedoraproject.org>
>>> ---
>>> This should let the protections for the eBPF work as expected, I don't
>>> know if there is some sort of self test for thatL.
>>
>>
>> This is going to conflict with my KASLR implementation, since it puts
>> the kernel image right in the middle of the vmalloc area, and the
>> kernel is obviously mapped with block mappings. In fact, I am
>> proposing enabling huge-vmap for arm64 as well, since it seems an
>> improvement generally, but also specifically allows me to unmap the
>> __init section using the generic vunmap code (remove_vm_area). But in
>> general, I think the assumption that the whole vmalloc area is mapped
>> using pages is not tenable.
>>
>> AFAICT, vmalloc still use pages exclusively even with huge-vmap (but
>> ioremap does not). So perhaps it would make sense to check for the
>> VM_ALLOC bit in the VMA flags (which I will not set for the kernel
>> regions either)
>>
>
> Something along these lines, perhaps?
>
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index 3571c7309c5e..bda0a776c58e 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -13,6 +13,7 @@
>   #include <linux/kernel.h>
>   #include <linux/mm.h>
>   #include <linux/module.h>
> +#include <linux/vmalloc.h>
>   #include <linux/sched.h>
>
>   #include <asm/pgtable.h>
> @@ -44,6 +45,7 @@ static int change_memory_common(unsigned long addr
>          unsigned long end = start + size;
>          int ret;
>          struct page_change_data data;
> +       struct vm_struct *area;
>
>          if (!PAGE_ALIGNED(addr)) {
>                  start &= PAGE_MASK;
> @@ -51,10 +53,14 @@ static int change_memory_common(unsigned long addr,
>                  WARN_ON_ONCE(1);
>          }
>
> -       if (start < MODULES_VADDR || start >= MODULES_END)
> -               return -EINVAL;
> -
> -       if (end < MODULES_VADDR || end >= MODULES_END)
> +       /*
> +        * Check whether the [addr, addr + size) interval is entirely
> +        * covered by precisely one VM area that has the VM_ALLOC flag set
> +        */
> +       area = find_vm_area((void *)addr);
> +       if (!area ||
> +           end > (unsigned long)area->addr + area->size ||
> +           !(area->flags & VM_ALLOC))
>                  return -EINVAL;
>
>          data.set_mask = set_mask;
>
>

The patch works in my test for both vmalloc and module spaces. Can we either
keep my comment or add another that explains why we are excluding the rest
of the kernel range?

Thanks,
Laura

  
>>
>>> ---
>>>   arch/arm64/mm/pageattr.c | 25 +++++++++++++++++++++----
>>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
>>> index 3571c73..274208e 100644
>>> --- a/arch/arm64/mm/pageattr.c
>>> +++ b/arch/arm64/mm/pageattr.c
>>> @@ -36,6 +36,26 @@ static int change_page_range(pte_t *ptep, pgtable_t token, unsigned long addr,
>>>          return 0;
>>>   }
>>>
>>> +static bool validate_addr(unsigned long start, unsigned long end)
>>> +{
>>> +       /*
>>> +        * This check explicitly excludes most kernel memory. Most kernel
>>> +        * memory is mapped with a larger page size and breaking down the
>>> +        * larger page size without causing TLB conflicts is very difficult.
>>> +        *
>>> +        * If you need to call set_memory_* on a range, the recommendation is
>>> +        * to use vmalloc since that range is mapped with pages.
>>> +        */
>>> +       if (start >= MODULES_VADDR && start < MODULES_END &&
>>> +           end >= MODULES_VADDR && end < MODULES_END)
>>> +               return true;
>>> +
>>> +       if (is_vmalloc_addr(start) && is_vmalloc_addr(end))
>>> +               return true;
>>> +
>>> +       return false;
>>> +}
>>> +
>>>   static int change_memory_common(unsigned long addr, int numpages,
>>>                                  pgprot_t set_mask, pgprot_t clear_mask)
>>>   {
>>> @@ -51,10 +71,7 @@ static int change_memory_common(unsigned long addr, int numpages,
>>>                  WARN_ON_ONCE(1);
>>>          }
>>>
>>> -       if (start < MODULES_VADDR || start >= MODULES_END)
>>> -               return -EINVAL;
>>> -
>>> -       if (end < MODULES_VADDR || end >= MODULES_END)
>>> +       if (!validate_addr(start, end))
>>>                  return -EINVAL;
>>>
>>>          data.set_mask = set_mask;
>>> --
>>> 2.5.0
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web