Re: [PATCH v3 1/1] mm: fix use-after-free when anon vma name is used after vma is freed

From: Michal Hocko <mhocko@suse.com>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Colin Cross <ccross@google.com>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Dave Hansen <dave.hansen@intel.com>,
	Kees Cook <keescook@chromium.org>,
	Matthew Wilcox <willy@infradead.org>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Johannes Weiner <hannes@cmpxchg.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Christian Brauner <brauner@kernel.org>,
	legion@kernel.org, ran.xiaokai@zte.com.cn, sashal@kernel.org,
	Chris Hyser <chris.hyser@oracle.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Peter Collingbourne <pcc@google.com>,
	caoxiaofeng@yulong.com, David Hildenbrand <david@redhat.com>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	linux-mm <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	kernel-team <kernel-team@android.com>,
	syzbot+aa7b3d4b35f9dc46a366@syzkaller.appspotmail.com
Subject: Re: [PATCH v3 1/1] mm: fix use-after-free when anon vma name is used after vma is freed
Date: Wed, 16 Feb 2022 09:06:52 +0100	[thread overview]
Message-ID: <YgywnF8l4Zu0aLtF@dhcp22.suse.cz> (raw)
In-Reply-To: <CAJuCfpGG9zwbvfH5UZkt6cG=woeO0RGE7QxjEpXn=gFhiaDdmQ@mail.gmail.com>

On Tue 15-02-22 15:02:54, Suren Baghdasaryan wrote:
> On Tue, Feb 15, 2022 at 12:05 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > One thing I was considering is to check agains ref counte overflo (a
> > deep process chain with many vmas could grow really high. ref_count
> > interface doesn't provide any easy way to check for overflows as far as
> > I could see from a quick glance so I gave up there but the logic would
> > be really straightforward. We just create a new anon_vma_name with the same
> > content and use it when duplicating if the usage grow really
> > (arbitrarily) high.
> 
> I went over proposed changes. I see a couple small required fixes
> (resetting the name to NULL seems to be missing and I think
> dup_vma_anon_name needs some tweaking) but overall quite
> straight-forward.

OK, great that this makes sense to you. As I've said I didn't really go
into details, not even dared to boot that to test. So it will very
likely need some more work but I do not expect this to grow much.

> I'll post a separate patch to do this refactoring.
> The original patch is fixing the UAF issue, so I don't want to mix it
> with refactoring. Please let me know if you see an issue with
> separating it that way.

Well, I am not sure TBH. Look at diffstats. Your fix
2 files changed, 63 insertions(+), 17 deletions(-)
the refactoring which should fix this and potentially others that might
be still lurking there (because mixing shared pointers and their internal
objects just begs for problems) is
7 files changed, 63 insertions(+), 86 deletions(-)

more files touched for sure but the net result is much more clear and a
much more code removed.
The overflow logic would make it bigger but I guess the existing scheme
needs it as well.

I would also claim that both approaches are really painful to review
because the existing model spreads into several areas and it is not
really clear you caught them all just by staring into the diff so both
will be rather painful to backport to older kernels. Fortunately this
would be only 5.17.
-- 
Michal Hocko
SUSE Labs