Re: crypto: Kernel memory overwrite attempt detected to spans multiple pages

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Apr 10, 2019 at 11:30:57AM -0700, Kees Cook wrote:
> On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Thu, Mar 21, 2019 at 10:51:22AM -0700, Eric Biggers wrote:
> > > On Thu, Mar 21, 2019 at 10:45:31AM -0700, Kees Cook wrote:
> > > > On Wed, Mar 20, 2019 at 11:57 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > On Tue, Mar 19, 2019 at 10:09:13AM -0700, Eric Biggers wrote:
> > > > > > On Tue, Mar 19, 2019 at 12:54:23PM +0100, Geert Uytterhoeven wrote:
> > > > > > > When running the sha1-asm crypto selftest on arm with
> > > > > > > CONFIG_HARDENED_USERCOPY_PAGESPAN=y:
> > > > > > >
> > > > > > >     usercopy: Kernel memory overwrite attempt detected to spans
> > > > > > > multiple pages (offset 0, size 42)!
> > > > > >
> > > > > > Well, this must happen with the new (in 5.1) crypto self-tests implementation
> > > > > > for any crypto algorithm when CONFIG_HARDENED_USERCOPY_PAGESPAN=y.  I don't
> > > > > > understand why hardened usercopy considers it a bug though, as there's no buffer
> > > > > > overflow.  The crypto tests use copy_from_iter() to copy data into a 2-page
> > > > > > buffer that was allocated with __get_free_pages():
> > > > > >
> > > > > >       __get_free_pages(GFP_KERNEL, 1)
> > > > > >
> > > > > > ... where 1 means an order-1 allocation.
> > > > > >
> > > > > > If it copies to offset=4064 len=42, for example, then hardened usercopy
> > > > > > considers it a bug even though the buffer is 8192 bytes long.  Why?
> > > > > >
> > > > > > It isn't actually copying anything to/from userspace, BTW; it's using iov_iter
> > > > > > with ITER_KVEC.
> > > > > >
> > > > > > - Eric
> > > > >
> > > > > Kees, any thoughts on why hardened usercopy rejects copies spanning a page
> > > > > boundary when they seem to be fine?
> > > >
> > > > This is due to missing the compound page marking, if I remember
> > > > correctly. However, I tend to leave the pagespan test disabled: it
> > > > really isn't ready for production use -- there are a lot of missing
> > > > annotations still.
> > > >
> > >
> > > So do I need to add __GFP_COMP?  Is there any actual reason to do so?
> > > Why does hardened usercopy check for it?
> > >
> > > - Eric
> >
> > Hi Kees, any answer to this question?
> 
> Hi! Sorry, this got lost in my inbox. Yes, if you can add __GFP_COMP,
> that would fix this case. No one has had time lately to track down all
> these cases, but avoiding adding new ones would be wonderful. :)
> 
> It's in there because it's a state I'd like to get to in the kernel,
> but it'll require a lot more work to get there.
> 

That didn't answer my question.  My question is what is the purpose of this?  If
there was actual buffer overflow when __GFP_COMP isn't specified that would make
perfect sense, but AFAICS there isn't.  So why does hardened usercopy consider
it broken when __GFP_COMP isn't specified?

- Eric