| Message ID | 20211004224224.4137992-4-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Assorted improvements to usercopy | expand |
On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote: > Move the compound page overrun detection out of > CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/usercopy.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/mm/usercopy.c b/mm/usercopy.c > index 7bfc4f9ed1e4..e395462961d5 100644 > --- a/mm/usercopy.c > +++ b/mm/usercopy.c > @@ -194,11 +194,6 @@ static inline void check_page_span(const void *ptr, unsigned long n, > ((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 > @@ -250,6 +245,10 @@ 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 if (PageHead(page)) { > + /* A compound allocation */ > + if (ptr + n > page_address(page) + page_size(page)) > + usercopy_abort("page alloc", NULL, to_user, 0, n); "0" could be "ptr - page_address(page)", I think? With that: Acked-by: Kees Cook <keescook@chromium.org> -Kees > } else { > /* Verify object does not incorrectly span multiple pages. */ > check_page_span(ptr, n, page, to_user); > -- > 2.32.0 >
On Tue, Oct 05, 2021 at 02:26:37PM -0700, Kees Cook wrote: > On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote: > > + } else if (PageHead(page)) { > > + /* A compound allocation */ > > + if (ptr + n > page_address(page) + page_size(page)) > > + usercopy_abort("page alloc", NULL, to_user, 0, n); > > "0" could be "ptr - page_address(page)", I think? With that: > > Acked-by: Kees Cook <keescook@chromium.org> Right, so that can be: } else if (PageHead(page)) { /* A compound allocation */ unsigned long offset = ptr - page_address(page); if (offset + n > page_size(page)) usercopy_abort("page alloc", NULL, to_user, offset, n); which saves us calling page_address() twice. Probably GCC is smart enough to CSE it anyway, but it also avoids splitting at the 80 column boundary ;-)
On Tue, Oct 05, 2021 at 11:12:47PM +0100, Matthew Wilcox wrote: > On Tue, Oct 05, 2021 at 02:26:37PM -0700, Kees Cook wrote: > > On Mon, Oct 04, 2021 at 11:42:23PM +0100, Matthew Wilcox (Oracle) wrote: > > > + } else if (PageHead(page)) { > > > + /* A compound allocation */ > > > + if (ptr + n > page_address(page) + page_size(page)) > > > + usercopy_abort("page alloc", NULL, to_user, 0, n); > > > > "0" could be "ptr - page_address(page)", I think? With that: > > > > Acked-by: Kees Cook <keescook@chromium.org> > > Right, so that can be: > > } else if (PageHead(page)) { > /* A compound allocation */ > unsigned long offset = ptr - page_address(page); > if (offset + n > page_size(page)) > usercopy_abort("page alloc", NULL, to_user, offset, n); > > which saves us calling page_address() twice. Probably GCC is smart > enough to CSE it anyway, but it also avoids splitting at the 80 column > boundary ;-) Perfect, yes!
diff --git a/mm/usercopy.c b/mm/usercopy.c index 7bfc4f9ed1e4..e395462961d5 100644 --- a/mm/usercopy.c +++ b/mm/usercopy.c @@ -194,11 +194,6 @@ static inline void check_page_span(const void *ptr, unsigned long n, ((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 @@ -250,6 +245,10 @@ 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 if (PageHead(page)) { + /* A compound allocation */ + if (ptr + n > page_address(page) + page_size(page)) + usercopy_abort("page alloc", NULL, to_user, 0, n); } else { /* Verify object does not incorrectly span multiple pages. */ check_page_span(ptr, n, page, to_user);
Move the compound page overrun detection out of CONFIG_HARDENED_USERCOPY_PAGESPAN so it's enabled for more people. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/usercopy.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)