On Thu, Sep 12, 2024 at 02:34:54PM +0500, Muhammad Usama Anjum wrote:
> On 9/12/24 11:36 AM, Muhammad Usama Anjum wrote:
> > Hi Dan,
> >
> > Thank you for reporting.
> I've debugged more and found out that no changes are required as
> access_ok() already deals well with the overflows. I've tested the
> corner cases on x86_64 and there are no issue found.
>
> I'll add more test cases in the selftest for this ioctl. Please share
> your thoughts if I may have missed something.
>
I don't understand what you are saying. We are discussing three issues:
1) We check the size and then make the size larger:
check: if (!access_ok((void __user *)(long)arg->start, arg->end - arg->start))
larger: arg->end = ALIGN(arg->end, PAGE_SIZE);
The ALIGN() can't make ->end go beyond the end of the page so it's possible that
if you have access_ok to the start of the page then you have access to the whole
page. It just seems ugly to rely on that. (I don't know mm very well).
2) Passing negative sizes to access_ok().
The access_ok() check will treat negative sizes as very large positive sizes and
it will be rejected. So far as I can see this is fine. Plus there is a check
at the start of walk_page_range() which says if (start >= end) return -EINVAL;
3) Integer overflow:
access_ok((void __user *)(long)arg->vec, arg->vec_len * sizeof(struct page_region))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This multiplication can overflow so access_ok() checks a smaller size than
intended. It will absolutely return success when it shouldn't. To determine
the impact then we have to look at do_pagemap_scan() but I don't know the code
well enough to say if it's harmless or what the impact is.
Integer overflows to access_ok() are always treated as a bug though even if it's
harmless.
regards,
dan carpenter
