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

[Pratyush Anand]

On Tuesday 14 February 2017 03:55 AM, Kees Cook wrote:
> On Mon, Jan 30, 2017 at 11:00 AM, Pratyush Anand <panand@xxxxxxxxxx> wrote:
> > CCing Andrew and Kees for their review comments.
> >
> >
> > On Wednesday 25 January 2017 10:14 AM, Pratyush Anand wrote:
> > > 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 <panand@xxxxxxxxxx>
> > > ---
> > > 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