Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
On Fri, May 10, 2019 at 01:43:36PM -0700, 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 > Signed-off-by: Kees Cook So, after looking at this more, I think I'm going to keep this patch, and we can add new sanity checks on a per-Page flag check. (See below.) Andrew, can you apply this to -mm please? > --- > 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; > - > - /* 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); > - } > -#endif > -} > - > static inline void check_heap_object(const void *ptr, unsigned long n, >bool to_user) > { > @@ -236,9 +172,6 @@ static inline void check_heap_object(const void *ptr, > unsigned long n, > if (PageSlab(page)) { > /* Check slab allocator for flags and size. */ > __check_heap_object(ptr, n, page, to_user); > - } else { > - /* Verify object does not incorrectly span multiple pages. */ > - check_page_span(ptr, n, page, to_user); > } In the future, instead of this catch-all "else", we can add things like: } else if (PageCompound(page)) { ... do some check for compound pages ... } else if (PageReserved(page)) ... etc ... } But for 5.3, I think we need to just entirely drop the PAGESPAN thing. -Kees > } >
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
On Mon, Jun 10, 2019 at 03:30:55PM -0700, Eric Biggers wrote: > Any progress on this patch? I have no had time yet; sorry. If anyone else would like to take a stab at it, I'd appreciate it. :) -- Kees Cook
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
On Mon, May 13, 2019 at 02:32:43PM -0700, Kees Cook wrote: > On Sat, May 11, 2019 at 09:11:42PM -0700, Matthew Wilcox wrote: > > On Sat, May 11, 2019 at 05:03:08PM -0700, Kees Cook wrote: > > > 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 > > > > > > > > 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; > > > > > > 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'd delete that first check, because it's a subset of the second check, > > It seemed easier to short-circuit with a math test before doing the slightly > more expensive virt_to_head_page(end) call. Do you think that's sensible? > > > > > /* Is the object wholly within a single (base or compound) page? */ > > endpage = virt_to_head_page(end); > > if (likely(endpage == page)) > > return; > > > > /* > > * If the start and end are more than MAX_ORDER apart, they must > > * be from separate allocations > > */ > > if (n >= (PAGE_SIZE << MAX_ORDER)) > > usercopy_abort("spans too many pages", NULL, to_user, 0, n); > > > > /* > > * If neither page is compound, we can't tell if the object is > > * within a single allocation or not > > */ > > if (!PageCompound(page) && !PageCompound(endpage)) > > return; > > > > > I really wish we had size of allocation reliably held somewhere. We'll > > > need it for doing memory tagging of the page allocator too... > > > > I think we'll need to store those allocations in a separate data structure > > on the side. As far as the rest of the kernel is concerned, those struct > > pages belong to them once the page allocator has given them. > > Okay, let me work up a page-type refactoring while allocation size can > stay back-burnered. > > -- > Kees Cook Any progress on this patch? - Eric
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
On Sat, May 11, 2019 at 09:11:42PM -0700, Matthew Wilcox wrote: > On Sat, May 11, 2019 at 05:03:08PM -0700, Kees Cook wrote: > > 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 > > > > > > 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; > > > > 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'd delete that first check, because it's a subset of the second check, It seemed easier to short-circuit with a math test before doing the slightly more expensive virt_to_head_page(end) call. Do you think that's sensible? > > /* Is the object wholly within a single (base or compound) page? */ > endpage = virt_to_head_page(end); > if (likely(endpage == page)) > return; > > /* >* If the start and end are more than MAX_ORDER apart, they must >* be from separate allocations >*/ > if (n >= (PAGE_SIZE << MAX_ORDER)) > usercopy_abort("spans too many pages", NULL, to_user, 0, n); > > /* >* If neither page is compound, we can't tell if the object is >* within a single allocation or not >*/ > if (!PageCompound(page) && !PageCompound(endpage)) > return; > > > I really wish we had size of allocation reliably held somewhere. We'll > > need it for doing memory tagging of the page allocator too... > > I think we'll need to store those allocations in a separate data structure > on the side. As far as the rest of the kernel is concerned, those struct > pages belong to them once the page allocator has given them. Okay, let me work up a page-type refactoring while allocation size can stay back-burnered. -- Kees Cook
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
On Sat, May 11, 2019 at 05:03:08PM -0700, Kees Cook wrote: > 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 > > > > 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; > > 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'd delete that first check, because it's a subset of the second check, /* Is the object wholly within a single (base or compound) page? */ endpage = virt_to_head_page(end); if (likely(endpage == page)) return; /* * If the start and end are more than MAX_ORDER apart, they must * be from separate allocations */ if (n >= (PAGE_SIZE << MAX_ORDER)) usercopy_abort("spans too many pages", NULL, to_user, 0, n); /* * If neither page is compound, we can't tell if the object is * within a single allocation or not */ if (!PageCompound(page) && !PageCompound(endpage)) return; > I really wish we had size of allocation reliably held somewhere. We'll > need it for doing memory tagging of the page allocator too... I think we'll need to store those allocations in a separate data structure on the side. As far as the rest of the kernel is concerned, those struct pages belong to them once the page allocator has given them.
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
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 > > Signed-off-by: Kees Cook > > --- > > 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
Re: [PATCH] usercopy: Remove HARDENED_USERCOPY_PAGESPAN
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 Signed-off-by: Kees Cook --- 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.? - /* 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); - } -#endif -} - static inline void check_heap_object(const void *ptr, unsigned long n, bool to_user) { @@ -236,9 +172,6 @@ static inline void check_heap_object(const void *ptr, unsigned long n, if (PageSlab(page)) { /* Check slab allocator for flags and size. */ __check_heap_object(ptr, n, page, to_user); - } else { - /* Verify object does not incorrectly span multiple pages. */ - check_page_span(ptr, n, page, to_user); } } diff --git a/security/Kconfig b/security/Kconfig index 353cfef71d4e..8392647f5a4c 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -176,17 +176,6 @@ config HARDENED_USERCOPY_FALLBACK Booting with "slab_common.usercopy_fallback=Y/N" can change this setting. -config HARDENED_USERCOPY_PAGESPAN - bool "Refuse to copy allocations that span multiple pages" - depends on HARDENED_USERCOPY - depends on EXPERT - help - When a multi-page allocation is done with