Re: CVE-2024-36000: mm/hugetlb: fix missing hugetlb_lock for resv uncharge

[Michal Hocko]

Let me add Oscar,

On Tue 21-05-24 15:38:45, Peter Xu wrote:
> On Mon, May 20, 2024 at 05:14:58PM +0200, Michal Hocko wrote:
> > Peter,
> 
> Hi, Michal,
> 
> > does b76b46902c2d ("mm/hugetlb: fix missing hugetlb_lock for resv
> > uncharge") really have any security implications? I fail to see any but
> > UFFD is not really my area so I might be missing something very easily.
> 
> AFAIU that issue wasn't userfault specific, but a generic issue for hugetlb
> - I believe that can also trigger in other paths whoever try to call
> alloc_hugetlb_folio(), and UFFDIO_COPY is one user of it.
> 
> I looked at that and provided a fix only because the report originated from
> the uffd report, so Andrew normally pointing those to me, and since I
> looked anyway I tried to fix that.

OK, I see. Thanks for the clarification.

> Here in general what I can see is that the lock is needed since this
> commit:
> 
>     commit 94ae8ba7176666d1e7d8bbb9f93670a27540b6a8
>     Author: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
>     Date:   Tue Jul 31 16:42:35 2012 -0700
> 
>     hugetlb/cgroup: assign the page hugetlb cgroup when we move the page to active list.
> 
> That commit mentioned that we rely on the lock to make sure all hugetlb
> folios on the active list will have a valid memcg.  However I'm not sure
> whether it's still required now (after all that's 2012..), e.g., I'm
> looking at hugetlb_cgroup_css_offline(), and hugetlb_cgroup_move_parent()
> looks all safe to even take empty memcg folios with the latest code at
> least:
> 
> 	/*
> 	 * We can have pages in active list without any cgroup
> 	 * ie, hugepage with less than 3 pages. We can safely
> 	 * ignore those pages.
> 	 */
> 	if (!page_hcg || page_hcg != h_cg)
> 		goto out;
> 
> In short, I don't know any further security implications on this problem
> besides LOCKDEP enabled.  But I don't think I fully understand the hugetlb
> reservation code, so please just take that with a grain of salt.  E.g.,
> right now we do the hugetlb_cgroup_uncharge_folio_rsvd(), then could it
> happen that this folio will still be used finally and got injected into the
> pgtables (after all, alloc_hugetlb_folio() will still return this folio to
> the caller with a success), and would that be a problem if this folio has
> its _hugetlb_cgroup_rsvd==NULL?  That looks like another question besides
> this specific problem, though..

Oscar, could you have a look please?

-- 
Michal Hocko
SUSE Labs