| Message ID | 20190411192827.72551-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto: testmgr - allocate buffers with __GFP_COMP | expand |
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 >
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
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.
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,
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.
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 ...
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.
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.
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.
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; }