Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 11 May 2019 17:03:08 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Laura Abbott <labbott@redhat.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Eric Biggers <ebiggers@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Rik van Riel <riel@surriel.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
Message-ID: <201905111657.76FE0BDCEC@keescook>
References: <201905101341.A17DDD7@keescook>
 <ead5e4ad-d911-3680-04a4-ae98507ba48c@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ead5e4ad-d911-3680-04a4-ae98507ba48c@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 10, 2019 at 08:41:43PM -0400, Laura Abbott wrote:
> On 5/10/19 3:43 PM, Kees Cook wrote:
> > This feature continues to cause more problems than it solves[1]. Its
> > intention was to check the bounds of page-allocator allocations by using
> > __GFP_COMP, for which we would need to find all missing __GFP_COMP
> > markings. This work has been on hold and there is an argument[2]
> > that such markings are not even the correct signal for checking for
> > same-allocation pages. Instead of depending on BROKEN, this just removes
> > it entirely. It can be trivially reverted if/when a better solution for
> > tracking page allocator sizes is found.
> > 
> > [1] https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg37479.html
> > [2] https://lkml.kernel.org/r/20190415022412.GA29714@bombadil.infradead.org
> > 
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >   mm/usercopy.c    | 67 ------------------------------------------------
> >   security/Kconfig | 11 --------
> >   2 files changed, 78 deletions(-)
> > 
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index 14faadcedd06..15dc1bf03303 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -159,70 +159,6 @@ static inline void check_bogus_address(const unsigned long ptr, unsigned long n,
> >   		usercopy_abort("null address", NULL, to_user, ptr, n);
> >   }
> > -/* Checks for allocs that are marked in some way as spanning multiple pages. */
> > -static inline void check_page_span(const void *ptr, unsigned long n,
> > -				   struct page *page, bool to_user)
> > -{
> > -#ifdef CONFIG_HARDENED_USERCOPY_PAGESPAN
> > -	const void *end = ptr + n - 1;
> > -	struct page *endpage;
> > -	bool is_reserved, is_cma;
> > -
> > -	/*
> > -	 * Sometimes the kernel data regions are not marked Reserved (see
> > -	 * check below). And sometimes [_sdata,_edata) does not cover
> > -	 * rodata and/or bss, so check each range explicitly.
> > -	 */
> > -
> > -	/* Allow reads of kernel rodata region (if not marked as Reserved). */
> > -	if (ptr >= (const void *)__start_rodata &&
> > -	    end <= (const void *)__end_rodata) {
> > -		if (!to_user)
> > -			usercopy_abort("rodata", NULL, to_user, 0, n);
> > -		return;
> > -	}
> > -
> > -	/* Allow kernel data region (if not marked as Reserved). */
> > -	if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
> > -		return;
> > -
> > -	/* Allow kernel bss region (if not marked as Reserved). */
> > -	if (ptr >= (const void *)__bss_start &&
> > -	    end <= (const void *)__bss_stop)
> > -		return;
> > -
> 
> 
> I agree the page spanning is broken but is it worth keeping the
> checks against __rodata __bss etc.?

They're all just white-listing later checks (except RODATA which is
doing a cheap RO test which is redundant on any architecture with actual
rodata...) so they don't have any value in staying without the rest of
the page allocator logic.

> 
> > -	/* Is the object wholly within one base page? */
> > -	if (likely(((unsigned long)ptr & (unsigned long)PAGE_MASK) ==
> > -		   ((unsigned long)end & (unsigned long)PAGE_MASK)))
> > -		return;
> > -
> > -	/* Allow if fully inside the same compound (__GFP_COMP) page. */
> > -	endpage = virt_to_head_page(end);
> > -	if (likely(endpage == page))
> > -		return;
> > -
> > -	/*
> > -	 * Reject if range is entirely either Reserved (i.e. special or
> > -	 * device memory), or CMA. Otherwise, reject since the object spans
> > -	 * several independently allocated pages.
> > -	 */
> > -	is_reserved = PageReserved(page);
> > -	is_cma = is_migrate_cma_page(page);
> > -	if (!is_reserved && !is_cma)
> > -		usercopy_abort("spans multiple pages", NULL, to_user, 0, n);
> > -
> > -	for (ptr += PAGE_SIZE; ptr <= end; ptr += PAGE_SIZE) {
> > -		page = virt_to_head_page(ptr);
> > -		if (is_reserved && !PageReserved(page))
> > -			usercopy_abort("spans Reserved and non-Reserved pages",
> > -				       NULL, to_user, 0, n);
> > -		if (is_cma && !is_migrate_cma_page(page))
> > -			usercopy_abort("spans CMA and non-CMA pages", NULL,
> > -				       to_user, 0, n);
> > -	}

We _could_ keep the mixed CMA/reserved/neither check if we really wanted
to, but that's such a corner case of a corner case, I'm not sure it's
worth doing the virt_to_head_page() across the whole span to figure
it out.

I really wish we had size of allocation reliably held somewhere. We'll
need it for doing memory tagging of the page allocator too...

-- 
Kees Cook