crypto: testmgr - allocate buffers with __GFP_COMP
diff mbox series

Message ID 20190411192827.72551-1-ebiggers@kernel.org
State New
Headers show
Series
  • crypto: testmgr - allocate buffers with __GFP_COMP
Related show

Commit Message

Eric Biggers April 11, 2019, 7:28 p.m. UTC
From: Eric Biggers <ebiggers@google.com>

This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
incorrectly report a buffer overflow when the destination of
copy_from_iter() spans the page boundary in the 2-page buffer.

Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/testmgr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kees Cook April 11, 2019, 8:32 p.m. UTC | #1
On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
> incorrectly report a buffer overflow when the destination of
> copy_from_iter() spans the page boundary in the 2-page buffer.
>
> Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

> ---
>  crypto/testmgr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 0f6bfb6ce6a46..3522c0bed2492 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
>         int i;
>
>         for (i = 0; i < XBUFSIZE; i++) {
> -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> +                                                 order);

Is there a reason __GFP_COMP isn't automatically included in all page
allocations? (Or rather, it seems like the exception is when things
should NOT be considered part of the same allocation, so something
like __GFP_SINGLE should exist?.)

-Kees

>                 if (!buf[i])
>                         goto err_free_buf;
>         }
> --
> 2.21.0.392.gf8f6787159e-goog
>
Dmitry Vyukov April 12, 2019, 5:38 a.m. UTC | #2
On Thu, Apr 11, 2019 at 10:32 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 12:31 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > This is needed so that CONFIG_HARDENED_USERCOPY_PAGESPAN=y doesn't
> > incorrectly report a buffer overflow when the destination of
> > copy_from_iter() spans the page boundary in the 2-page buffer.
> >
> > Fixes: 3f47a03df6e8 ("crypto: testmgr - add testvec_config struct and helper functions")
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> > ---
> >  crypto/testmgr.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> > index 0f6bfb6ce6a46..3522c0bed2492 100644
> > --- a/crypto/testmgr.c
> > +++ b/crypto/testmgr.c
> > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> >         int i;
> >
> >         for (i = 0; i < XBUFSIZE; i++) {
> > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > +                                                 order);
>
> Is there a reason __GFP_COMP isn't automatically included in all page
> allocations? (Or rather, it seems like the exception is when things
> should NOT be considered part of the same allocation, so something
> like __GFP_SINGLE should exist?.)

It would be reasonable if __get_free_pages would automatically mark
consecutive pages as consecutive.
When these should not be considered part of the same allocation? Is it
possible to free them separately? Will that BUG with __GFP_COMP mark?
I understand that there can be a weak "these are actually the same
allocation, but I would like to think about them as separate". But
potentially we can ignore such cases.

> -Kees
>
> >                 if (!buf[i])
> >                         goto err_free_buf;
> >         }
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
Matthew Wilcox April 15, 2019, 2:24 a.m. UTC | #3
On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> >         int i;
> >
> >         for (i = 0; i < XBUFSIZE; i++) {
> > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > +                                                 order);
> 
> Is there a reason __GFP_COMP isn't automatically included in all page
> allocations? (Or rather, it seems like the exception is when things
> should NOT be considered part of the same allocation, so something
> like __GFP_SINGLE should exist?.)

The question is not whether or not things should be considered part of the
same allocation.  The question is whether the allocation is of a compound
page or of N consecutive pages.  Now you're asking what the difference is,
and it's whether you need to be able to be able to call compound_head(),
compound_order(), PageTail() or use a compound_dtor.  If you don't, then
you can save some time at allocation & free by not specifying __GFP_COMP.

I'll agree this is not documented well, and maybe most multi-page
allocations do want __GFP_COMP and we should invert that bit, but
__GFP_SINGLE doesn't seem like the right antonym to __GFP_COMP to me.
Herbert Xu April 15, 2019, 2:46 a.m. UTC | #4
On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote:
> On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> > >         int i;
> > >
> > >         for (i = 0; i < XBUFSIZE; i++) {
> > > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > > +                                                 order);
> > 
> > Is there a reason __GFP_COMP isn't automatically included in all page
> > allocations? (Or rather, it seems like the exception is when things
> > should NOT be considered part of the same allocation, so something
> > like __GFP_SINGLE should exist?.)
> 
> The question is not whether or not things should be considered part of the
> same allocation.  The question is whether the allocation is of a compound
> page or of N consecutive pages.  Now you're asking what the difference is,
> and it's whether you need to be able to be able to call compound_head(),
> compound_order(), PageTail() or use a compound_dtor.  If you don't, then
> you can save some time at allocation & free by not specifying __GFP_COMP.

Thanks for clarifying Matthew.

Eric, this means that we should not use __GFP_COMP here just to
silent what is clearly a broken warning.

Cheers,
Matthew Wilcox April 16, 2019, 2:18 a.m. UTC | #5
On Mon, Apr 15, 2019 at 10:46:15AM +0800, Herbert Xu wrote:
> On Sun, Apr 14, 2019 at 07:24:12PM -0700, Matthew Wilcox wrote:
> > On Thu, Apr 11, 2019 at 01:32:32PM -0700, Kees Cook wrote:
> > > > @@ -156,7 +156,8 @@ static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
> > > >         int i;
> > > >
> > > >         for (i = 0; i < XBUFSIZE; i++) {
> > > > -               buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
> > > > +               buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
> > > > +                                                 order);
> > > 
> > > Is there a reason __GFP_COMP isn't automatically included in all page
> > > allocations? (Or rather, it seems like the exception is when things
> > > should NOT be considered part of the same allocation, so something
> > > like __GFP_SINGLE should exist?.)
> > 
> > The question is not whether or not things should be considered part of the
> > same allocation.  The question is whether the allocation is of a compound
> > page or of N consecutive pages.  Now you're asking what the difference is,
> > and it's whether you need to be able to be able to call compound_head(),
> > compound_order(), PageTail() or use a compound_dtor.  If you don't, then
> > you can save some time at allocation & free by not specifying __GFP_COMP.
> 
> Thanks for clarifying Matthew.
> 
> Eric, this means that we should not use __GFP_COMP here just to
> silent what is clearly a broken warning.

I agree; if the crypto code is never going to try to go from the address of
a byte in the allocation back to the head page, then there's no need to
specify GFP_COMP.

But that leaves us in the awkward situation where
HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
'ptr + n - 1' lies within the same allocation as ptr.  Without using
a compound page, there's no indication in the VM structures that these
two pages were allocated as part of the same allocation.

We could force all multi-page allocations to be compound pages if
HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
something.  We could make it catch fewer problems by succeeding if the
page is not compound.  I don't know, these all seem like bad choices
to me.
Kees Cook April 16, 2019, 3:14 a.m. UTC | #6
On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> I agree; if the crypto code is never going to try to go from the address of
> a byte in the allocation back to the head page, then there's no need to
> specify GFP_COMP.
>
> But that leaves us in the awkward situation where
> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> a compound page, there's no indication in the VM structures that these
> two pages were allocated as part of the same allocation.
>
> We could force all multi-page allocations to be compound pages if
> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> something.  We could make it catch fewer problems by succeeding if the
> page is not compound.  I don't know, these all seem like bad choices
> to me.

If GFP_COMP is _not_ the correct signal about adjacent pages being
part of the same allocation, then I agree: we need to drop this check
entirely from PAGESPAN. Is there anything else that indicates this
property? (Or where might we be able to store that info?)

There are other pagespan checks, though, so those could stay. But I'd
really love to gain page allocator allocation size checking ...
Matthew Wilcox April 17, 2019, 4:08 a.m. UTC | #7
On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > I agree; if the crypto code is never going to try to go from the address of
> > a byte in the allocation back to the head page, then there's no need to
> > specify GFP_COMP.
> >
> > But that leaves us in the awkward situation where
> > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> > a compound page, there's no indication in the VM structures that these
> > two pages were allocated as part of the same allocation.
> >
> > We could force all multi-page allocations to be compound pages if
> > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > something.  We could make it catch fewer problems by succeeding if the
> > page is not compound.  I don't know, these all seem like bad choices
> > to me.
> 
> If GFP_COMP is _not_ the correct signal about adjacent pages being
> part of the same allocation, then I agree: we need to drop this check
> entirely from PAGESPAN. Is there anything else that indicates this
> property? (Or where might we be able to store that info?)

As far as I know, the page allocator does not store size information
anywhere, unless you use GFP_COMP.  That's why you have to pass
the 'order' to free_pages() and __free_pages().  It's also why
alloc_pages_exact() works (follow all the way into split_page()).

> There are other pagespan checks, though, so those could stay. But I'd
> really love to gain page allocator allocation size checking ...

I think that's a great idea, but I'm not sure how you'll be able to
do that.
Russell King - ARM Linux admin April 17, 2019, 8:09 a.m. UTC | #8
On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
> > On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > I agree; if the crypto code is never going to try to go from the address of
> > > a byte in the allocation back to the head page, then there's no need to
> > > specify GFP_COMP.
> > >
> > > But that leaves us in the awkward situation where
> > > HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
> > > 'ptr + n - 1' lies within the same allocation as ptr.  Without using
> > > a compound page, there's no indication in the VM structures that these
> > > two pages were allocated as part of the same allocation.
> > >
> > > We could force all multi-page allocations to be compound pages if
> > > HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
> > > something.  We could make it catch fewer problems by succeeding if the
> > > page is not compound.  I don't know, these all seem like bad choices
> > > to me.
> > 
> > If GFP_COMP is _not_ the correct signal about adjacent pages being
> > part of the same allocation, then I agree: we need to drop this check
> > entirely from PAGESPAN. Is there anything else that indicates this
> > property? (Or where might we be able to store that info?)
> 
> As far as I know, the page allocator does not store size information
> anywhere, unless you use GFP_COMP.  That's why you have to pass
> the 'order' to free_pages() and __free_pages().  It's also why
> alloc_pages_exact() works (follow all the way into split_page()).
> 
> > There are other pagespan checks, though, so those could stay. But I'd
> > really love to gain page allocator allocation size checking ...
> 
> I think that's a great idea, but I'm not sure how you'll be able to
> do that.

However, we have had code (maybe historically now) that has allocated
a higher order page and then handed back pages that it doesn't need -
for example, when the code requires multiple contiguous pages but does
not require a power-of-2 size of contiguous pages.
Robin Murphy April 17, 2019, 9:54 a.m. UTC | #9
On 17/04/2019 09:09, Russell King - ARM Linux admin wrote:
> On Tue, Apr 16, 2019 at 09:08:22PM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 15, 2019 at 10:14:51PM -0500, Kees Cook wrote:
>>> On Mon, Apr 15, 2019 at 9:18 PM Matthew Wilcox <willy@infradead.org> wrote:
>>>> I agree; if the crypto code is never going to try to go from the address of
>>>> a byte in the allocation back to the head page, then there's no need to
>>>> specify GFP_COMP.
>>>>
>>>> But that leaves us in the awkward situation where
>>>> HARDENED_USERCOPY_PAGESPAN does need to be able to figure out whether
>>>> 'ptr + n - 1' lies within the same allocation as ptr.  Without using
>>>> a compound page, there's no indication in the VM structures that these
>>>> two pages were allocated as part of the same allocation.
>>>>
>>>> We could force all multi-page allocations to be compound pages if
>>>> HARDENED_USERCOPY_PAGESPAN is enabled, but I worry that could break
>>>> something.  We could make it catch fewer problems by succeeding if the
>>>> page is not compound.  I don't know, these all seem like bad choices
>>>> to me.
>>>
>>> If GFP_COMP is _not_ the correct signal about adjacent pages being
>>> part of the same allocation, then I agree: we need to drop this check
>>> entirely from PAGESPAN. Is there anything else that indicates this
>>> property? (Or where might we be able to store that info?)
>>
>> As far as I know, the page allocator does not store size information
>> anywhere, unless you use GFP_COMP.  That's why you have to pass
>> the 'order' to free_pages() and __free_pages().  It's also why
>> alloc_pages_exact() works (follow all the way into split_page()).
>>
>>> There are other pagespan checks, though, so those could stay. But I'd
>>> really love to gain page allocator allocation size checking ...
>>
>> I think that's a great idea, but I'm not sure how you'll be able to
>> do that.
> 
> However, we have had code (maybe historically now) that has allocated
> a higher order page and then handed back pages that it doesn't need -
> for example, when the code requires multiple contiguous pages but does
> not require a power-of-2 size of contiguous pages.

'git grep alloc_pages_exact' suggests it's not historical yet...

Robin.

Patch
diff mbox series

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 0f6bfb6ce6a46..3522c0bed2492 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -156,7 +156,8 @@  static int __testmgr_alloc_buf(char *buf[XBUFSIZE], int order)
 	int i;
 
 	for (i = 0; i < XBUFSIZE; i++) {
-		buf[i] = (char *)__get_free_pages(GFP_KERNEL, order);
+		buf[i] = (char *)__get_free_pages(GFP_KERNEL | __GFP_COMP,
+						  order);
 		if (!buf[i])
 			goto err_free_buf;
 	}