BUG: /proc/kcore does not export direct-mapped memory on arm64 (and presumably some other architectures)

PATCH In review Dave Anderson 4 participants 8 messages Apr 26, 2018 – May 1, 2018 1 message not in the archive

Subsystems (3)

Memory management Proc filesystem Kcore

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dave Anderson

author

2018-04-26 19:31 UTC #1 lore raw

While testing /proc/kcore as the live memory source for the crash utility, it fails on arm64. The failure on arm64 occurs because only the vmalloc/module space segments are exported in PT_LOAD segments, and it's missing all of the PT_LOAD segments for the generic unity-mapped regions of physical memory, as well as their associated vmemmap sections.

The mapping of unity-mapped RAM segments in fs/proc/kcore.c is architecture-neutral, and after debugging it, I found this as the problem. For each chunk of physical memory, kcore_update_ram() calls walk_system_ram_range(), passing kclist_add_private() as a callback function to add the chunk to the kclist, and eventually leading to the creation of a PT_LOAD segment.

kclist_add_private() does some verification of the memory region, but this one below is bogus for arm64:

    static int
    kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg)
    {
    ... [ cut ] ...
            ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
    ... [ cut ] ...
  
            /* Sanity check: Can happen in 32bit arch...maybe */
            if (ent->addr < (unsigned long) __va(0))
                    goto free_out;

And that's because __va(0) is a bogus check for arm64. It is checking whether the ent->addr value is less than the lowest possible unity-mapped address. But "0" should not be used as a physical address on arm64; the lowest legitimate physical address for this __va() check would be the arm64 PHYS_OFFSET, or memstart_addr:

Here's the arm64 __va() and PHYS_OFFSET:

  #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
  #define __phys_to_virt(x)       ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)

extern s64 memstart_addr;
/* PHYS_OFFSET - the physical address of the start of memory. */
#define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; })

If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000 on my test system), the __va(0) calculation goes negative and creates a bogus, very large, virtual address. And since the ent->addr virtual address is less than bogus __va(0) address, the test fails, and the memory chunk is rejected.

Looking at the kernel sources, it seems that this would affect other architectures as well, i.e., the ones whose __va() is not a simple addition of the physical address with PAGE_OFFSET.

Anyway, I don't know what the best approach for an architecture-neutral fix would be in this case. So I figured I'd throw it out to you guys for some ideas.

Dave Anderson

Kees Cook

2018-04-26 21:16 UTC #2 lore raw

Quoted 49 lines · Dave Anderson

While testing /proc/kcore as the live memory source for the crash utility, it fails on arm64. The failure on arm64 occurs because only the vmalloc/module space segments are exported in PT_LOAD segments, and it's missing all of the PT_LOAD segments for the generic unity-mapped regions of physical memory, as well as their associated vmemmap sections.
The mapping of unity-mapped RAM segments in fs/proc/kcore.c is architecture-neutral, and after debugging it, I found this as the problem. For each chunk of physical memory, kcore_update_ram() calls walk_system_ram_range(), passing kclist_add_private() as a callback function to add the chunk to the kclist, and eventually leading to the creation of a PT_LOAD segment.
kclist_add_private() does some verification of the memory region, but this one below is bogus for arm64:
    static int
    kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg)
    {
    ... [ cut ] ...
            ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
    ... [ cut ] ...

            /* Sanity check: Can happen in 32bit arch...maybe */
            if (ent->addr < (unsigned long) __va(0))
                    goto free_out;
And that's because __va(0) is a bogus check for arm64. It is checking whether the ent->addr value is less than the lowest possible unity-mapped address. But "0" should not be used as a physical address on arm64; the lowest legitimate physical address for this __va() check would be the arm64 PHYS_OFFSET, or memstart_addr:
Here's the arm64 __va() and PHYS_OFFSET:
  #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
  #define __phys_to_virt(x)       ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)
extern s64 memstart_addr;
/* PHYS_OFFSET - the physical address of the start of memory. */
#define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; })
If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000 on my test system), the __va(0) calculation goes negative and creates a bogus, very large, virtual address. And since the ent->addr virtual address is less than bogus __va(0) address, the test fails, and the memory chunk is rejected.

Quoted 7 lines

Looking at the kernel sources, it seems that this would affect other architectures as well, i.e., the ones whose __va() is not a simple addition of the physical address with PAGE_OFFSET.
Anyway, I don't know what the best approach for an architecture-neutral fix would be in this case. So I figured I'd throw it out to you guys for some ideas.

I'm not as familiar with this code, but I've added Ard and Laura to CC here, as this feels like something they'd be able to comment on. :)

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Laura Abbott

2018-04-28 00:58 UTC #3 lore raw

Quoted 54 lines · Kees Cook

Quoted 53 lines · Dave Anderson
While testing /proc/kcore as the live memory source for the crash utility, it fails on arm64. The failure on arm64 occurs because only the vmalloc/module space segments are exported in PT_LOAD segments, and it's missing all of the PT_LOAD segments for the generic unity-mapped regions of physical memory, as well as their associated vmemmap sections.
The mapping of unity-mapped RAM segments in fs/proc/kcore.c is architecture-neutral, and after debugging it, I found this as the problem. For each chunk of physical memory, kcore_update_ram() calls walk_system_ram_range(), passing kclist_add_private() as a callback function to add the chunk to the kclist, and eventually leading to the creation of a PT_LOAD segment.
kclist_add_private() does some verification of the memory region, but this one below is bogus for arm64:
     static int
     kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg)
     {
     ... [ cut ] ...
             ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
     ... [ cut ] ...

             /* Sanity check: Can happen in 32bit arch...maybe */
             if (ent->addr < (unsigned long) __va(0))
                     goto free_out;
And that's because __va(0) is a bogus check for arm64. It is checking whether the ent->addr value is less than the lowest possible unity-mapped address. But "0" should not be used as a physical address on arm64; the lowest legitimate physical address for this __va() check would be the arm64 PHYS_OFFSET, or memstart_addr:
Here's the arm64 __va() and PHYS_OFFSET:
   #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
   #define __phys_to_virt(x)       ((unsigned long)((x) - PHYS_OFFSET) | PAGE_OFFSET)
   extern s64                      memstart_addr;
   /* PHYS_OFFSET - the physical address of the start of memory. */
   #define PHYS_OFFSET             ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; })
If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000 on my test system), the __va(0) calculation goes negative and creates a bogus, very large, virtual address. And since the ent->addr virtual address is less than bogus __va(0) address, the test fails, and the memory chunk is rejected.
Looking at the kernel sources, it seems that this would affect other architectures as well, i.e., the ones whose __va() is not a simple addition of the physical address with PAGE_OFFSET.

Quoted 9 lines

Quoted 3 lines
Anyway, I don't know what the best approach for an architecture-neutral fix would be in this case. So I figured I'd throw it out to you guys for some ideas.
I'm not as familiar with this code, but I've added Ard and Laura to CC here, as this feels like something they'd be able to comment on. :)
-Kees

It seems backwards that we're converting a physical address to a virtual address and then validating that. I think checking against pfn_valid (to ensure there is a valid memmap entry)
and then checking page_to_virt against virt_addr_valid to catch other cases (e.g. highmem or holes in the space) seems cleaner. Maybe something like:

fs/proc/kcore.c +16 -7

index d1e82761de81..e64ecb9f2720 100644
@@ -209,25 +209,34 @@ kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg)
 {
 	struct list_head *head = (struct list_head *)arg;
 	struct kcore_list *ent;
+	struct page *p;
+
+	if (!pfn_valid(pfn))
+		return 1;
+
+	p = pfn_to_page(pfn);
+	if (!memmap_valid_within(pfn, p, page_zone(p)))
+		return 1;
 
 	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
 	if (!ent)
 		return -ENOMEM;
-	ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
+	ent->addr = (unsigned long)page_to_virt(p);
 	ent->size = nr_pages << PAGE_SHIFT;
 
-	/* Sanity check: Can happen in 32bit arch...maybe */
-	if (ent->addr < (unsigned long) __va(0))
+	if (!virt_addr_valid(ent->addr))
 		goto free_out;
 
 	/* cut not-mapped area. ....from ppc-32 code. */
 	if (ULONG_MAX - ent->addr < ent->size)
 		ent->size = ULONG_MAX - ent->addr;
 
-	/* cut when vmalloc() area is higher than direct-map area */
-	if (VMALLOC_START > (unsigned long)__va(0)) {
-		if (ent->addr > VMALLOC_START)
-			goto free_out;
+	/*
+	 * We've already checked virt_addr_valid so we know this address
+	 * is a valid pointer, therefore we can check against it to determine
+	 * if we need to trim
+	 */
+	if (VMALLOC_START > ent->addr) {
 		if (VMALLOC_START - ent->addr < ent->size)
 			ent->size = VMALLOC_START - ent->addr;
 	}

Dave Anderson

author

2018-04-30 14:03 UTC #4 lore raw

----- Original Message -----

Quoted 70 lines · Kees Cook

Quoted 69 lines
Quoted 56 lines · Dave Anderson
While testing /proc/kcore as the live memory source for the crash utility, it fails on arm64. The failure on arm64 occurs because only the vmalloc/module space segments are exported in PT_LOAD segments, and it's missing all of the PT_LOAD segments for the generic unity-mapped regions of physical memory, as well as their associated vmemmap sections.
The mapping of unity-mapped RAM segments in fs/proc/kcore.c is architecture-neutral, and after debugging it, I found this as the problem. For each chunk of physical memory, kcore_update_ram() calls walk_system_ram_range(), passing kclist_add_private() as a callback function to add the chunk to the kclist, and eventually leading to the creation of a PT_LOAD segment.
kclist_add_private() does some verification of the memory region, but this one below is bogus for arm64:
     static int
     kclist_add_private(unsigned long pfn, unsigned long nr_pages, void
     *arg)
     {
     ... [ cut ] ...
             ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
     ... [ cut ] ...

             /* Sanity check: Can happen in 32bit arch...maybe */
             if (ent->addr < (unsigned long) __va(0))
                     goto free_out;
And that's because __va(0) is a bogus check for arm64. It is checking whether the ent->addr value is less than the lowest possible unity-mapped address. But "0" should not be used as a physical address on arm64; the lowest legitimate physical address for this __va() check would be the arm64
PHYS_OFFSET, or memstart_addr:
Here's the arm64 __va() and PHYS_OFFSET:
   #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
   #define __phys_to_virt(x)       ((unsigned long)((x) - PHYS_OFFSET) |
   PAGE_OFFSET)
   extern s64                      memstart_addr;
   /* PHYS_OFFSET - the physical address of the start of memory. */
   #define PHYS_OFFSET             ({ VM_BUG_ON(memstart_addr & 1);
   memstart_addr; })
If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000 on my
test system), the __va(0) calculation goes negative and creates a bogus, very
large, virtual address. And since the ent->addr virtual address is less than
bogus __va(0) address, the test fails, and the memory chunk is rejected.
Quoted 7 lines
Looking at the kernel sources, it seems that this would affect other architectures as well, i.e., the ones whose __va() is not a simple addition of the physical address with PAGE_OFFSET.
Anyway, I don't know what the best approach for an architecture-neutral fix would be in this case. So I figured I'd throw it out to you guys for some ideas.
I'm not as familiar with this code, but I've added Ard and Laura to CC here, as this feels like something they'd be able to comment on. :)

Quoted 8 lines

Quoted 2 lines
-Kees
It seems backwards that we're converting a physical address to a virtual address and then validating that. I think checking against pfn_valid (to ensure there is a valid memmap entry)
and then checking page_to_virt against virt_addr_valid to catch other cases (e.g. highmem or holes in the space) seems cleaner.

Hi Laura,

Thanks a lot for looking into this -- I couldn't find a maintainer for kcore.

The patch looks good to me, as long as virt_addr_valid() will fail on 32-bit arches when page_to_virt() creates an invalid address when it gets passed a highmem-physical address.

Thanks again,
Dave

Quoted 51 lines

Maybe something like:

fs/proc/kcore.c +16 -7

index d1e82761de81..e64ecb9f2720 100644
@@ -209,25 +209,34 @@ kclist_add_private(unsigned long pfn, unsigned long
 nr_pages, void *arg)
  {
  	struct list_head *head = (struct list_head *)arg;
  	struct kcore_list *ent;
+	struct page *p;
+
+	if (!pfn_valid(pfn))
+		return 1;
+
+	p = pfn_to_page(pfn);
+	if (!memmap_valid_within(pfn, p, page_zone(p)))
+		return 1;
 
  	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
  	if (!ent)
  		return -ENOMEM;
-	ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
+	ent->addr = (unsigned long)page_to_virt(p);
  	ent->size = nr_pages << PAGE_SHIFT;
 
-	/* Sanity check: Can happen in 32bit arch...maybe */
-	if (ent->addr < (unsigned long) __va(0))
+	if (!virt_addr_valid(ent->addr))
  		goto free_out;
 
  	/* cut not-mapped area. ....from ppc-32 code. */
  	if (ULONG_MAX - ent->addr < ent->size)
  		ent->size = ULONG_MAX - ent->addr;
 
-	/* cut when vmalloc() area is higher than direct-map area */
-	if (VMALLOC_START > (unsigned long)__va(0)) {
-		if (ent->addr > VMALLOC_START)
-			goto free_out;
+	/*
+	 * We've already checked virt_addr_valid so we know this address
+	 * is a valid pointer, therefore we can check against it to determine
+	 * if we need to trim
+	 */
+	if (VMALLOC_START > ent->addr) {
  		if (VMALLOC_START - ent->addr < ent->size)
  			ent->size = VMALLOC_START - ent->addr;

Dave Anderson

author

2018-05-01 14:45 UTC #5 lore raw

----- Original Message -----

Quoted 88 lines

----- Original Message -----
Quoted 74 lines · Kees Cook
Quoted 73 lines
Quoted 59 lines · Dave Anderson
While testing /proc/kcore as the live memory source for the crash utility,
it fails on arm64. The failure on arm64 occurs because only the vmalloc/module space segments are exported in PT_LOAD segments, and it's missing all of the PT_LOAD segments for the generic unity-mapped regions of physical memory, as well as their associated vmemmap sections.
The mapping of unity-mapped RAM segments in fs/proc/kcore.c is architecture-neutral, and after debugging it, I found this as the problem. For each chunk of physical memory, kcore_update_ram() calls walk_system_ram_range(), passing kclist_add_private() as a callback function to add the chunk to the kclist, and eventually leading to the creation of a PT_LOAD segment.
kclist_add_private() does some verification of the memory region, but this one below is bogus for arm64:
     static int
     kclist_add_private(unsigned long pfn, unsigned long nr_pages, void
     *arg)
     {
     ... [ cut ] ...
             ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
     ... [ cut ] ...

             /* Sanity check: Can happen in 32bit arch...maybe */
             if (ent->addr < (unsigned long) __va(0))
                     goto free_out;
And that's because __va(0) is a bogus check for arm64. It is checking whether the ent->addr value is less than the lowest possible unity-mapped
address. But "0" should not be used as a physical address on arm64; the lowest legitimate physical address for this __va() check would be the arm64
PHYS_OFFSET, or memstart_addr:
Here's the arm64 __va() and PHYS_OFFSET:
   #define __va(x) ((void *)__phys_to_virt((phys_addr_t)(x)))
   #define __phys_to_virt(x)       ((unsigned long)((x) - PHYS_OFFSET) |
   PAGE_OFFSET)
   extern s64                      memstart_addr;
   /* PHYS_OFFSET - the physical address of the start of memory. */
   #define PHYS_OFFSET             ({ VM_BUG_ON(memstart_addr & 1);
   memstart_addr; })
If PHYS_OFFSET/memstart_addr is anything other than 0 (it is 0x4000000000
on my
test system), the __va(0) calculation goes negative and creates a bogus, very
large, virtual address. And since the ent->addr virtual address is less than
bogus __va(0) address, the test fails, and the memory chunk is rejected.
Quoted 8 lines
Looking at the kernel sources, it seems that this would affect other architectures as well, i.e., the ones whose __va() is not a simple addition of the physical address with PAGE_OFFSET.
Anyway, I don't know what the best approach for an architecture-neutral fix would be in this case. So I figured I'd throw it out to you guys for
some ideas.
I'm not as familiar with this code, but I've added Ard and Laura to CC here, as this feels like something they'd be able to comment on. :)
Quoted 8 lines
Quoted 2 lines
-Kees
It seems backwards that we're converting a physical address to a virtual address and then validating that. I think checking against pfn_valid (to ensure there is a valid memmap entry)
and then checking page_to_virt against virt_addr_valid to catch other cases (e.g. highmem or holes in the space) seems cleaner.
Hi Laura,

Quoted 8 lines

Thanks a lot for looking into this -- I couldn't find a maintainer for kcore.
The patch looks good to me, as long as virt_addr_valid() will fail on 32-bit arches when page_to_virt() creates an invalid address when it gets passed a highmem-physical address.
Thanks again,
Dave

Laura,

Tested OK on x86_64, s390x, ppc64le and arm64. Note that the patch below had a cut-and-paste error -- the patch I used is attached.

Thanks,
Dave

Quoted 38 lines

Maybe something like:

fs/proc/kcore.c +10 -3

index d1e82761de81..e64ecb9f2720 100644
@@ -209,25 +209,34 @@ kclist_add_private(unsigned long pfn, unsigned long
 nr_pages, void *arg)
  {
  	struct list_head *head = (struct list_head *)arg;
  	struct kcore_list *ent;
+	struct page *p;
+
+	if (!pfn_valid(pfn))
+		return 1;
+
+	p = pfn_to_page(pfn);
+	if (!memmap_valid_within(pfn, p, page_zone(p)))
+		return 1;
 
  	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
  	if (!ent)
  		return -ENOMEM;
-	ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
+	ent->addr = (unsigned long)page_to_virt(p);
  	ent->size = nr_pages << PAGE_SHIFT;
 
-	/* Sanity check: Can happen in 32bit arch...maybe */
-	if (ent->addr < (unsigned long) __va(0))
+	if (!virt_addr_valid(ent->addr))
  		goto free_out;
 
  	/* cut not-mapped area. ....from ppc-32 code. */
  	if (ULONG_MAX - ent->addr < ent->size)
  		ent->size = ULONG_MAX - ent->addr;

Quoted 16 lines

Quoted 12 lines

fs/proc/kcore.c +6 -4

-	/* cut when vmalloc() area is higher than direct-map area */
-	if (VMALLOC_START > (unsigned long)__va(0)) {
-		if (ent->addr > VMALLOC_START)
-			goto free_out;
+	/*
+	 * We've already checked virt_addr_valid so we know this address
+	 * is a valid pointer, therefore we can check against it to determine
+	 * if we need to trim
+	 */
+	if (VMALLOC_START > ent->addr) {
  		if (VMALLOC_START - ent->addr < ent->size)
  			ent->size = VMALLOC_START - ent->addr;

Quoted 3 lines

fs/proc/kcore.c renamed from fs/proc/kcore.c.orig +19 -9

@@ -209,28 +209,38 @@ kclist_add_private(unsigned long pfn, un
 {
 	struct list_head *head = (struct list_head *)arg;
 	struct kcore_list *ent;
+	struct page *p;
+
+	if (!pfn_valid(pfn))
+		return 1;
+
+	p = pfn_to_page(pfn);
+	if (!memmap_valid_within(pfn, p, page_zone(p)))
+		return 1;
 
 	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
 	if (!ent)
 		return -ENOMEM;
-	ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
+	ent->addr = (unsigned long)page_to_virt(p);
 	ent->size = nr_pages << PAGE_SHIFT;
 
 	/* Sanity check: Can happen in 32bit arch...maybe */
-	if (ent->addr < (unsigned long) __va(0))
+	if (!virt_addr_valid(ent->addr))
 		goto free_out;
 
 	/* cut not-mapped area. ....from ppc-32 code. */
 	if (ULONG_MAX - ent->addr < ent->size)
 		ent->size = ULONG_MAX - ent->addr;
 
-	/* cut when vmalloc() area is higher than direct-map area */
-	if (VMALLOC_START > (unsigned long)__va(0)) {
-		if (ent->addr > VMALLOC_START)
-			goto free_out;
-		if (VMALLOC_START - ent->addr < ent->size)
-			ent->size = VMALLOC_START - ent->addr;
-	}
+	/*
+	 * We've already checked virt_addr_valid so we know this address
+	 * is a valid pointer, therefore we can check against it to determine
+	 * if we need to trim
+	 */
+	if (VMALLOC_START > ent->addr) {
+  		if (VMALLOC_START - ent->addr < ent->size)
+  			ent->size = VMALLOC_START - ent->addr;
+  	}
 
 	ent->type = KCORE_RAM;
 	list_add_tail(&ent->list, head);

Laura Abbott

2018-05-01 20:11 UTC #6 lore raw

PATCH 1/1 proc/kcore: Don't bounds check against address 0

The existing kcore code checks for bad addresses against __va(0) with the assumption that this is the lowest address on the system. This may not hold true on some systems (e.g. arm64) and produce overflows and crashes. Switch to using other functions to validate the address range.

Tested-by Dave Anderson <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
I took your previous comments as a tested by, please let me know if that was wrong. This should probably just go through -mm. I don't think this is necessary for stable but I can request it later if necessary.

---
 fs/proc/kcore.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

fs/proc/kcore.c +16 -7

index d1e82761de81..e64ecb9f2720 100644
@@ -209,25 +209,34 @@ kclist_add_private(unsigned long pfn, unsigned long nr_pages, void *arg)
 {
 	struct list_head *head = (struct list_head *)arg;
 	struct kcore_list *ent;
+	struct page *p;
+
+	if (!pfn_valid(pfn))
+		return 1;
+
+	p = pfn_to_page(pfn);
+	if (!memmap_valid_within(pfn, p, page_zone(p)))
+		return 1;
 
 	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
 	if (!ent)
 		return -ENOMEM;
-	ent->addr = (unsigned long)__va((pfn << PAGE_SHIFT));
+	ent->addr = (unsigned long)page_to_virt(p);
 	ent->size = nr_pages << PAGE_SHIFT;
 
-	/* Sanity check: Can happen in 32bit arch...maybe */
-	if (ent->addr < (unsigned long) __va(0))
+	if (!virt_addr_valid(ent->addr))
 		goto free_out;
 
 	/* cut not-mapped area. ....from ppc-32 code. */
 	if (ULONG_MAX - ent->addr < ent->size)
 		ent->size = ULONG_MAX - ent->addr;
 
-	/* cut when vmalloc() area is higher than direct-map area */
-	if (VMALLOC_START > (unsigned long)__va(0)) {
-		if (ent->addr > VMALLOC_START)
-			goto free_out;
+	/*
+	 * We've already checked virt_addr_valid so we know this address
+	 * is a valid pointer, therefore we can check against it to determine
+	 * if we need to trim
+	 */
+	if (VMALLOC_START > ent->addr) {
 		if (VMALLOC_START - ent->addr < ent->size)
 			ent->size = VMALLOC_START - ent->addr;
 	}

Signature · 1 line

2.14.3

Andrew Morton

maintainer

2018-05-01 21:46 UTC #7 lore raw

Re: [PATCH] proc/kcore: Don't bounds check against address 0

Quoted 6 lines · On Tue, 1 May 2018 13:11:43 -0700 Laura Abbott <[email protected]> wrote:

The existing kcore code checks for bad addresses against __va(0) with the assumption that this is the lowest address on the system. This may not hold true on some systems (e.g. arm64) and produce overflows and crashes. Switch to using other functions to validate the address range.

Quoted 6 lines

Tested-by Dave Anderson <[email protected]>
Signed-off-by Laura Abbott <[email protected]>
---
I took your previous comments as a tested by, please let me know if that was wrong. This should probably just go through -mm. I don't think this is necessary for stable but I can request it later if necessary.

I'm surprised. "overflows and crashes" sounds rather serious??

Laura Abbott

2018-05-01 22:26 UTC #8 lore raw

Re: [PATCH] proc/kcore: Don't bounds check against address 0

Quoted 8 lines · Andrew Morton

Quoted 6 lines · On Tue, 1 May 2018 13:11:43 -0700 Laura Abbott <[email protected]> wrote:
The existing kcore code checks for bad addresses against __va(0) with the assumption that this is the lowest address on the system. This may not hold true on some systems (e.g. arm64) and produce overflows and crashes. Switch to using other functions to validate the address range.

Quoted 9 lines

Quoted 6 lines
Tested-by Dave Anderson <[email protected]>
Signed-off-by Laura Abbott <[email protected]>
---
I took your previous comments as a tested by, please let me know if that was wrong. This should probably just go through -mm. I don't think this is necessary for stable but I can request it later if necessary.
I'm surprised. "overflows and crashes" sounds rather serious??

It's currently only seen on arm64 and it's not clear if anyone wants to use that particular combination on a stable release. I think a better phrase is "this is not urgent for stable".

Thanks,
Laura