On Tue, Apr 9, 2019 at 8:17 PM Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxxx> 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. -- Kees Cook
