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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170808231535.GA20840@redhat.com>
Date:   Tue, 8 Aug 2017 19:15:36 -0400
From:   Jerome Glisse <jglisse@...hat.com>
To:     Igor Stoppa <igor.stoppa@...wei.com>
Cc:     Michal Hocko <mhocko@...nel.org>, Linux-MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-security-module@...r.kernel.org,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        Kees Cook <keescook@...gle.com>
Subject: Re: [RFC] Tagging of vmalloc pages for supporting the pmalloc
 allocator

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