[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLQ9QzncM5PSYs49GqUDAqg7drTm9BT+k4c7XoDTQeVFA@mail.gmail.com>
Date: Fri, 24 Jun 2016 13:59:29 -0700
From: Kees Cook <keescook@...omium.org>
To: Rik van Riel <riel@...hat.com>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
Brad Spengler <spender@...ecurity.net>, PaX Team <pageexec@...email.hu>,
Casey Schaufler <casey.schaufler@...el.com>, Christoph Lameter <cl@...ux.com>,
Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RFC][PATCH v2 6/4] mm: disallow user copy to/from separately
allocated pages
On Fri, Jun 24, 2016 at 1:57 PM, Rik van Riel <riel@...hat.com> wrote:
> On Fri, 2016-06-24 at 13:53 -0700, Kees Cook wrote:
>> On Fri, Jun 10, 2016 at 12:44 PM, Rik van Riel <riel@...hat.com>
>> wrote:
>> >
>> > v2 of yesterday's patch, this one seems to completely work on my
>> > system, after taking _bdata & _edata into account.
>> >
>> > I am still looking into CMA, but am leaning towards doing that as
>> > a follow-up patch.
>> >
>> > ---8<---
>> >
>> > Subject: mm: disallow user copy to/from separately allocated pages
>> >
>> > A single copy_from_user or copy_to_user should go to or from a
>> > single
>> > kernel object. Inside the slab, or on the stack, we can track the
>> > individual objects.
>> >
>> > For the general kernel heap, we do not know exactly where each
>> > object
>> > is, but we can tell whether the whole range from ptr to ptr + n is
>> > inside the same page, or inside the same compound page.
>> >
>> > If the start and end of the "object" are in pages that were not
>> > allocated
>> > together, we are likely dealing with an overflow from one object
>> > into
>> > the next page, and should disallow this copy.
>> >
>> > Signed-off-by: Rik van Riel <riel@...hat.com>
>> > ---
>> > v2:
>> > - also test against _bdata & _edata, this appears to be necessary
>> > for
>> > some kernfs/sysfs stuff
>> > - clean up the code a little bit
>> >
>> > mm/usercopy.c | 27 +++++++++++++++++++++++----
>> > 1 file changed, 23 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/mm/usercopy.c b/mm/usercopy.c
>> > index e09c33070759..78869ea73194 100644
>> > --- a/mm/usercopy.c
>> > +++ b/mm/usercopy.c
>> > @@ -109,7 +109,8 @@ static inline bool
>> > check_kernel_text_object(const void *ptr, unsigned long n)
>> >
>> > static inline const char *check_heap_object(const void *ptr,
>> > unsigned long n)
>> > {
>> > - struct page *page;
>> > + struct page *page, *endpage;
>> > + const void *end = ptr + n - 1;
>> >
>> > if (ZERO_OR_NULL_PTR(ptr))
>> > return "<null>";
>> > @@ -118,11 +119,29 @@ static inline const char
>> > *check_heap_object(const void *ptr, unsigned long n)
>> > return NULL;
>> >
>> > page = virt_to_head_page(ptr);
>> > - if (!PageSlab(page))
>> > + if (PageSlab(page))
>> > + /* Check allocator for flags and size. */
>> > + return __check_heap_object(ptr, n, page);
>> > +
>> > + /* 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 NULL;
>> > +
>> > + /* Are the start and end inside the same compound page? */
>> > + endpage = virt_to_head_page(end);
>> > + if (likely(endpage == page))
>> > + return NULL;
>> > +
>> > + /* Is this a special area, eg. .rodata, .bss, or device
>> > memory? */
>> > + if (ptr >= (const void *)_sdata && end <= (const void
>> > *)_edata)
>> > + return NULL;
>> > +
>> > + if (PageReserved(page) && PageReserved(endpage))
>> > return NULL;
>> Shouldn't PageReserved cover the .data, .rodata, and .bss areas
>> already? Is the concern for the check being added here that a copy
>> might span multiple pages and that they're not allocated together
>> when
>> laying out the kernel data regions?
>
> Having just the PageReserved check was not enough, I had
> to specifically add the _sdata & _edata test to get things
> to work.
>
> It looks like PageReserved is simply not set in some cases,
> perhaps we are mapping those kernel sections with huge pages,
> but not setting up the compound page stuff for the backing
> pages, since they never pass through the buddy allocator?
Ah-ha, gotcha. Thanks!
-Kees
--
Kees Cook
Chrome OS & Brillo Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
