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.ga29...@bombadil.infradead.org
> >
> > Suggested-by: Eric Biggers <ebigg...@kernel.org>
> > Signed-off-by: Kees Cook <keesc...@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
