Received-SPF: pass (google.com: domain of linux-kernel+bounces-214848-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 14 Jun 2024 13:48:30 +0200
From: Oscar Salvador <osalvador@suse.de>
To: Peter Xu <peterx@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>, Oscar Salvador <OSalvador@suse.com>,
	cve@kernel.org, linux-kernel@vger.kernel.org,
	linux-cve-announce@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: CVE-2024-36000: mm/hugetlb: fix missing hugetlb_lock for resv
 uncharge
Message-ID: <ZmwuDvvTDpCFGTdi@localhost.localdomain>
References: <2024052023-CVE-2024-36000-cfc4@gregkh>
 <Zkto8rbtAUBql-78@tiehlicka>
 <Zkz4RRgfwUHPbQ5z@x1n>
 <Zk7ws6H0wwuiFAJW@tiehlicka>
 <Zk8bgS8IboS-7jQw@localhost.localdomain>
 <Zk9j5j-VSAOWrmg7@x1n>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zk9j5j-VSAOWrmg7@x1n>

On Thu, May 23, 2024 at 11:42:30AM -0400, Peter Xu wrote:
> I really don't know enough on these areas to tell, perhaps I missed
> something.  But maybe any of you may have some idea..  In general, I think
> besides LOCKDEP the lock is definitely needed to at least make sure things
> like:
> 
> 	__set_hugetlb_cgroup(folio, NULL, rsvd);

I do not think this is a problem, you are only setting folio->_hugetlb_cgroup_rsvd
to the hugetlb cgroup.
And no one else should fiddle with that folio.

> 	page_counter_uncharge(),

This on the hand might be another story:

 page_counter_uncharge
  new = atomic_long_sub_return(nr_pages, &counter->usage)
  propagate_protected_usage

The first atomic_long_sub_return is ok because it is an atomic one, so
whoever comes last will not see e.g: a half-updated value.
But propagate_protected_usage() is a bit more convoluted as involves a bunch of
atomic operations and comparasions that in case they are not serialized, the counters
will not be consistent, which means that any charge/uncharge operation that comes after
might not reflect reality.

So I guess we could end up with scenarios where cgroups would not get as many pages as
they should, or maybe more pages than they should.

If this reasoning is accurate, I am leaning towards taking this as a security fix.

-- 
Oscar Salvador
SUSE Labs