linux-kernel - Re: [PATCH 1/3] Protectable memory support

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: <20170706162742.GA2919@redhat.com>
Date:   Thu, 6 Jul 2017 12:27:43 -0400
From:   Jerome Glisse <jglisse@...hat.com>
To:     Igor Stoppa <igor.stoppa@...wei.com>
Cc:     keescook@...omium.org, mhocko@...nel.org, jmorris@...ei.org,
        labbott@...hat.com, hch@...radead.org,
        penguin-kernel@...ove.SAKURA.ne.jp, paul@...l-moore.com,
        sds@...ho.nsa.gov, casey@...aufler-ca.com,
        linux-security-module@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH 1/3] Protectable memory support

On Wed, Jul 05, 2017 at 04:46:26PM +0300, Igor Stoppa wrote:
> The MMU available in many systems running Linux can often provide R/O
> protection to the memory pages it handles.
> 
> However, the MMU-based protection works efficiently only when said pages
> contain exclusively data that will not need further modifications.
> 
> Statically allocated variables can be segregated into a dedicated
> section, but this does not sit very well with dynamically allocated ones.
> 
> Dynamic allocation does not provide, currently, any means for grouping
> variables in memory pages that would contain exclusively data suitable
> for conversion to read only access mode.
> 
> The allocator here provided (pmalloc - protectable memory allocator)
> introduces the concept of pools of protectable memory.
> 
> A module can request a pool and then refer any allocation request to the
> pool handler it has received.
> 
> Once all the chunks of memory associated to a specific pool are
> initialized, the pool can be protected.
> 
> After this point, the pool can only be destroyed (it is up to the module
> to avoid any further references to the memory from the pool, after
> the destruction is invoked).
> 
> The latter case is mainly meant for releasing memory, when a module is
> unloaded.
> 
> A module can have as many pools as needed, for example to support the
> protection of data that is initialized in sufficiently distinct phases.
> 
> Signed-off-by: Igor Stoppa <igor.stoppa@...wei.com>
> ---
>  arch/Kconfig                   |   1 +
>  include/linux/page-flags.h     |   2 +
>  include/linux/pmalloc.h        | 127 +++++++++++++++
>  include/trace/events/mmflags.h |   1 +
>  lib/Kconfig                    |   1 +
>  mm/Makefile                    |   1 +
>  mm/pmalloc.c                   | 356 +++++++++++++++++++++++++++++++++++++++++
>  mm/usercopy.c                  |  24 +--
>  8 files changed, 504 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/pmalloc.h
>  create mode 100644 mm/pmalloc.c
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 6c00e5b..9d16b51 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -228,6 +228,7 @@ config GENERIC_IDLE_POLL_SETUP
>  
>  # Select if arch has all set_memory_ro/rw/x/nx() functions in asm/cacheflush.h
>  config ARCH_HAS_SET_MEMORY
> +	select GENERIC_ALLOCATOR
>  	bool
>  
>  # Select if arch init_task initializer is different to init/init_task.c
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6b5818d..acc0723 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -81,6 +81,7 @@ enum pageflags {
>  	PG_active,
>  	PG_waiters,		/* Page has waiters, check its waitqueue. Must be bit #7 and in the same byte as "PG_locked" */
>  	PG_slab,
> +	PG_pmalloc,
>  	PG_owner_priv_1,	/* Owner use. If pagecache, fs may use*/
>  	PG_arch_1,
>  	PG_reserved,
> @@ -274,6 +275,7 @@ PAGEFLAG(Active, active, PF_HEAD) __CLEARPAGEFLAG(Active, active, PF_HEAD)
>  	TESTCLEARFLAG(Active, active, PF_HEAD)
>  __PAGEFLAG(Slab, slab, PF_NO_TAIL)
>  __PAGEFLAG(SlobFree, slob_free, PF_NO_TAIL)
> +__PAGEFLAG(Pmalloc, pmalloc, PF_NO_TAIL)
>  PAGEFLAG(Checked, checked, PF_NO_COMPOUND)	   /* Used by some filesystems */
>  
>  /* Xen */


So i don't think we want to waste a page flag on this. The struct 
page flags field is already full AFAIK (see page-flags-layout.h)

Moreover there is easier way to tag such page. So my understanding
is that pmalloc() is always suppose to be in vmalloc area. From
the look of it all you do is check that there is a valid page behind
the vmalloc vaddr and you check for the PG_malloc flag of that page.

Why do you need to check the PG_malloc flag for the page ? Isn't the
fact that there is a page behind the vmalloc vaddr enough ? If not
enough wouldn't checking the pte flags of the page enough ? ie if
the page is read only inside vmalloc than it would be for sure some
pmalloc area.

Other way to distinguish between regular vmalloc and pmalloc can be
to carveout a region of vmalloc for pmalloc purpose. Issue is that
it might be hard to find right size for such carveout.

Yet another way is to use some of the free struct page fields ie
when a page is allocated for vmalloc i think most of struct page
fields are unuse (mapping, index, lru, ...). It would be better
to use those rather than adding a page flag.


Everything else looks good to me, thought i am unsure on how much
useful such feature is but i am not familiar too much with security
side of thing.


Cheers,
Jérôme