[PATCH v2 02/11] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 14, 2016 at 6:41 PM, Balbir Singh <bsingharora at gmail.com> wrote:
> On Thu, Jul 14, 2016 at 09:04:18PM -0400, Rik van Riel wrote:
>> On Fri, 2016-07-15 at 09:20 +1000, Balbir Singh wrote:
>>
>> > > ==
>> > > +            ((unsigned long)end & (unsigned
>> > > long)PAGE_MASK)))
>> > > +         return NULL;
>> > > +
>> > > + /* Allow if start and end are inside the same compound
>> > > page. */
>> > > + endpage = virt_to_head_page(end);
>> > > + if (likely(endpage == page))
>> > > +         return NULL;
>> > > +
>> > > + /* Allow special areas, device memory, and sometimes
>> > > kernel data. */
>> > > + if (PageReserved(page) && PageReserved(endpage))
>> > > +         return NULL;
>> >
>> > If we came here, it's likely that endpage > page, do we need to check
>> > that only the first and last pages are reserved? What about the ones
>> > in
>> > the middle?
>>
>> I think this will be so rare, we can get away with just
>> checking the beginning and the end.
>>
>
> But do we want to leave a hole where an aware user space
> can try a longer copy_* to avoid this check? If it is unlikely
> should we just bite the bullet and do the check for the entire
> range?

I'd be okay with expanding the test -- it should be an extremely rare
situation already since the common Reserved areas (kernel data) will
have already been explicitly tested.

What's the best way to do "next page"? Should it just be:

for ( ; page <= endpage ; ptr += PAGE_SIZE, page = virt_to_head_page(ptr) ) {
    if (!PageReserved(page))
        return "<spans multiple pages>";
}

return NULL;

?


-- 
Kees Cook
Chrome OS & Brillo Security