[PATCH] /proc/kcore: Update physical address for kcore ram and text

[Pratyush Anand]

Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.

User space tools like makedumpfile needs to know physical address for
PT_LOAD segments of direct mapped regions. Therefore this patch updates
paddr for such regions. It also sets an invalid paddr (-1) for other
regions, so that user space tool can know whether a physical address
provided in PT_LOAD is correct or not.

Signed-off-by: Pratyush Anand <***@redhat.com>
---
fs/proc/kcore.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 0b80ad87b4d6..ea9f3d1ae830 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int nphdr, int dataoff)
phdr->p_flags = PF_R|PF_W|PF_X;
phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff;
phdr->p_vaddr = (size_t)m->addr;
- phdr->p_paddr = 0;
+ if (m->type == KCORE_RAM || m->type == KCORE_TEXT)
+ phdr->p_paddr = __pa(m->addr);
+ else
+ phdr->p_paddr = (elf_addr_t)-1;
phdr->p_filesz = phdr->p_memsz = m->size;
phdr->p_align = PAGE_SIZE;
}

[Dave Young]

Hi Pratyush

Post by Pratyush Anand
Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.

I do not know the history of /proc/kcore, so a question is why the
p_addr was set as 0, if there were some reasons and if this could cause
some risk or breakage.

Post by Pratyush Anand
User space tools like makedumpfile needs to know physical address for
PT_LOAD segments of direct mapped regions. Therefore this patch updates
paddr for such regions. It also sets an invalid paddr (-1) for other
regions, so that user space tool can know whether a physical address
provided in PT_LOAD is correct or not.
---
fs/proc/kcore.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 0b80ad87b4d6..ea9f3d1ae830 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int nphdr, int dataoff)
phdr->p_flags = PF_R|PF_W|PF_X;
phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff;
phdr->p_vaddr = (size_t)m->addr;
- phdr->p_paddr = 0;
+ if (m->type == KCORE_RAM || m->type == KCORE_TEXT)
+ phdr->p_paddr = __pa(m->addr);
+ else
+ phdr->p_paddr = (elf_addr_t)-1;
phdr->p_filesz = phdr->p_memsz = m->size;
phdr->p_align = PAGE_SIZE;
}
--
2.9.3

Thanks
Dave

[Pratyush Anand]

Hi Dave,

Post by Dave Young
Hi Pratyush

Post by Pratyush Anand
Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.

I do not know the history of /proc/kcore, so a question is why the
p_addr was set as 0, if there were some reasons and if this could cause
some risk or breakage.

I do not know why it was 0, which is a valid physical address. But
certainly, it might break some user space tools, and those need to be
fixed. For example, see following code from kexec-tools

kexec/kexec-elf.c:build_mem_phdrs()

435 if ((phdr->p_paddr + phdr->p_memsz) < phdr->p_paddr) {
436 /* The memory address wraps */
437 if (probe_debug) {
438 fprintf(stderr, "ELF address wrap
around\n");
439 }
440 return -1;
441 }

We do not need to perform above check for an invalid physical address.

I think, kexec-tools and makedumpfile will need fixup. I already have
those fixup which will be sent upstream once this patch makes through.
Pro with this approach is that, it will help to calculate variable like
page_offset, phys_base from PT_LOAD even when they are randomized and
therefore will reduce many variable and version specific values in user
space tools.

~Pratyush

[Kees Cook]

CCing Andrew and Kees for their review comments.

Post by Pratyush Anand
Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.
User space tools like makedumpfile needs to know physical address for
PT_LOAD segments of direct mapped regions. Therefore this patch updates
paddr for such regions. It also sets an invalid paddr (-1) for other
regions, so that user space tool can know whether a physical address
provided in PT_LOAD is correct or not.
---
fs/proc/kcore.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 0b80ad87b4d6..ea9f3d1ae830 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int nphdr, int dataoff)
phdr->p_flags = PF_R|PF_W|PF_X;
phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff;
phdr->p_vaddr = (size_t)m->addr;
- phdr->p_paddr = 0;
+ if (m->type == KCORE_RAM || m->type == KCORE_TEXT)
+ phdr->p_paddr = __pa(m->addr);
+ else
+ phdr->p_paddr = (elf_addr_t)-1;
phdr->p_filesz = phdr->p_memsz = m->size;
phdr->p_align = PAGE_SIZE;
}

Well, CONFIG_PROC_KCORE is a generalized root KASLR exposure (though
there are lots of such exposures). Why is the actual physical address
needed? Can this just report the virtual address instead? Then the
tool can build a map, but it looks like an identity map, rather than
creating a new physical/virtual memory ASLR offset exposure?

-Kees

[Pratyush Anand]

Post by Kees Cook

CCing Andrew and Kees for their review comments.

Post by Pratyush Anand
Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.
User space tools like makedumpfile needs to know physical address for
PT_LOAD segments of direct mapped regions. Therefore this patch updates
paddr for such regions. It also sets an invalid paddr (-1) for other
regions, so that user space tool can know whether a physical address
provided in PT_LOAD is correct or not.
---
fs/proc/kcore.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 0b80ad87b4d6..ea9f3d1ae830 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int
nphdr, int dataoff)
phdr->p_flags = PF_R|PF_W|PF_X;
phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff;
phdr->p_vaddr = (size_t)m->addr;
- phdr->p_paddr = 0;
+ if (m->type == KCORE_RAM || m->type == KCORE_TEXT)
+ phdr->p_paddr = __pa(m->addr);
+ else
+ phdr->p_paddr = (elf_addr_t)-1;
phdr->p_filesz = phdr->p_memsz = m->size;
phdr->p_align = PAGE_SIZE;
}

Well, CONFIG_PROC_KCORE is a generalized root KASLR exposure (though
there are lots of such exposures). Why is the actual physical address
needed? Can this just report the virtual address instead? Then the
tool can build a map, but it looks like an identity map, rather than
creating a new physical/virtual memory ASLR offset exposure?

Well, having an ASLR offset information can help to translate an
identity mapped virtual address to a physical address. But that would be
an additional field in PT_LOAD header structure and an arch dependent value.

Moreover, sending a valid physical address like 0 does not seem right.
So, IMHO it is better to fix that and send valid physical address when
available (identity mapped).

Thanks for the review.

~Pratyush

[Pratyush Anand]

Hi Andrew/Kees,

Post by Pratyush Anand

Post by Kees Cook
Well, CONFIG_PROC_KCORE is a generalized root KASLR exposure (though
there are lots of such exposures). Why is the actual physical address
needed? Can this just report the virtual address instead? Then the
tool can build a map, but it looks like an identity map, rather than
creating a new physical/virtual memory ASLR offset exposure?

Well, having an ASLR offset information can help to translate an
identity mapped virtual address to a physical address. But that would be
an additional field in PT_LOAD header structure and an arch dependent value.
Moreover, sending a valid physical address like 0 does not seem right.
So, IMHO it is better to fix that and send valid physical address when
available (identity mapped).
Thanks for the review.

So, whats the decision on this patch? I see that patch is lying in
next/master. Should I expect this patch in v4.11-rc1?

Couple of user-space makedumpfile modification will depend on this
patch. So, we can not get those makedumpfile patches merged until this
patch hits upstream.

~Pratyush

[Baoquan He]

CC Eric too.

Post by Kees Cook

CCing Andrew and Kees for their review comments.

Post by Pratyush Anand
Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is
not true and could be misleading, since 0 is a valid physical address.
User space tools like makedumpfile needs to know physical address for
PT_LOAD segments of direct mapped regions. Therefore this patch updates
paddr for such regions. It also sets an invalid paddr (-1) for other
regions, so that user space tool can know whether a physical address
provided in PT_LOAD is correct or not.
---
fs/proc/kcore.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 0b80ad87b4d6..ea9f3d1ae830 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int
nphdr, int dataoff)
phdr->p_flags = PF_R|PF_W|PF_X;
phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff;
phdr->p_vaddr = (size_t)m->addr;
- phdr->p_paddr = 0;
+ if (m->type == KCORE_RAM || m->type == KCORE_TEXT)
+ phdr->p_paddr = __pa(m->addr);
+ else
+ phdr->p_paddr = (elf_addr_t)-1;
phdr->p_filesz = phdr->p_memsz = m->size;
phdr->p_align = PAGE_SIZE;
}

Well, CONFIG_PROC_KCORE is a generalized root KASLR exposure (though
there are lots of such exposures). Why is the actual physical address
needed? Can this just report the virtual address instead? Then the
tool can build a map, but it looks like an identity map, rather than
creating a new physical/virtual memory ASLR offset exposure?

HPE asked to add a dumped vmcore size estimate feature to makedumpfile,
just like HP UNIX does. So I added a --mem-usage option to makedumpfile
and use /proc/kcore to analyze the memory of 1st kernel. Since /proc/kcore
is a elf file which contains the mm layout of 1st kernel, it can help us
estimate how much disk space need be reserved. Later s390x people also
add support for this feature.

With kaslr enabled, page_offset becomes uncertain, and in kernel Eric
doesn't suggest exporting them into vmcoreinfo. So Pratyush tried to add
physical address of direct mapping regions. For kaslr, kcore has
exported those randomized starting address, I am not sure if it's risky
to export physical address.

It's close to our rhel dev cycle deadline, we hope this can be merged
soon. I believe we can discuss it and improve it any time if risk is
felt.

Thanks
Baoquan