[kernel-hardening] Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc allocator

[Jerome Glisse <jglisse@redhat.com>]

On Tue, Aug 08, 2017 at 03:59:36PM +0300, Igor Stoppa wrote:
> On 07/08/17 22:12, Jerome Glisse wrote:
> > On Mon, Aug 07, 2017 at 05:13:00PM +0300, Igor Stoppa wrote:
> 
> [...]
> 
> >> I have an updated version of the old proposal:
> >>
> >> * put a magic number in the private field, during initialization of
> >> pmalloc pages
> >>
> >> * during hardened usercopy verification, when I have to assess if a page
> >> is of pmalloc type, compare the private field against the magic number
> >>
> >> * if and only if the private field matches the magic number, then invoke
> >> find_vm_area(), so that the slowness affects only a possibly limited
> >> amount of false positives.
> > 
> > This all sounds good to me.
> 
> ok, I still have one doubt wrt defining the flag.
> Where should I do it?
> 
> vmalloc.h has the following:
> 
> /* bits in flags of vmalloc's vm_struct below */
> #define VM_IOREMAP		0x00000001	/* ioremap() and friends
> 						*/
> #define VM_ALLOC		0x00000002	/* vmalloc() */
> #define VM_MAP			0x00000004	/* vmap()ed pages */
> #define VM_USERMAP		0x00000008	/* suitable for
> 						   remap_vmalloc_range
> 						*/
> #define VM_UNINITIALIZED	0x00000020	/* vm_struct is not
> 						   fully initialized */
> #define VM_NO_GUARD		0x00000040      /* don't add guard page
> 						*/
> #define VM_KASAN		0x00000080      /* has allocated kasan
> 						shadow memory */
> /* bits [20..32] reserved for arch specific ioremap internals */
> 
> 
> 
> I am tempted to add
> 
> #define VM_PMALLOC		0x00000100
> 
> But would it be acceptable, to mention pmalloc into vmalloc?
> 
> Should I name it VM_PRIVATE bit, instead?
> 
> Using VM_PRIVATE would avoid contaminating vmalloc with something that
> depends on it (like VM_PMALLOC would do).
> 
> But using VM_PRIVATE will likely add tracking issues, if someone else
> wants to use the same bit and it's not clear who is the user, if any.

VM_PMALLOC sounds fine to me also adding a comment there pointing to
pmalloc documentation would be a good thing to do. The above are flags
that are use only inside vmalloc context and so there is no issue
here of conflicting with other potential user.

> 
> Unless it's acceptable to check the private field in the page struct.
> It would bear the pmalloc magic number.

I thought you wanted to do:
  check struct page mapping field
  check vmap->flags for VM_PMALLOC

bool is_pmalloc(unsigned long addr)
{
    struct page *page;
    struct vm_struct *vm_struct;

    if (!is_vmalloc_addr(addr))
        return false;
    page = vmalloc_to_page(addr);
    if (!page)
        return false;
    if (page->mapping != pmalloc_magic_key)
        return false;

    vm_struct = find_vm_area(addr);
    if (!vm_struct)
        return false;

    return vm_struct->flags & VM_PMALLOC;
}

Did you change your plan ?

> 
> I'm thinking to use a pointer to one of pmalloc data items, as signature.

What ever is easier for you. Note that dereferencing such pointer before
asserting this is really a pmalloc page would be hazardous.

Jérôme