Re: [PATCH v2 05/18] xfs: Add xfs_break_layouts() to the inode eviction path

From: Jason Gunthorpe <jgg@nvidia.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	akpm@linux-foundation.org, Matthew Wilcox <willy@infradead.org>,
	Jan Kara <jack@suse.cz>, "Darrick J. Wong" <djwong@kernel.org>,
	Christoph Hellwig <hch@lst.de>,
	John Hubbard <jhubbard@nvidia.com>,
	linux-fsdevel@vger.kernel.org, nvdimm@lists.linux.dev,
	linux-xfs@vger.kernel.org, linux-mm@kvack.org,
	linux-ext4@vger.kernel.org
Subject: Re: [PATCH v2 05/18] xfs: Add xfs_break_layouts() to the inode eviction path
Date: Mon, 26 Sep 2022 10:04:33 -0300	[thread overview]
Message-ID: <YzGjYZGFBBWBfUbK@nvidia.com> (raw)
In-Reply-To: <20220926003430.GB3600936@dread.disaster.area>

On Mon, Sep 26, 2022 at 10:34:30AM +1000, Dave Chinner wrote:

> > It is not about sane applications, it is about kernel security against
> > hostile userspace.
> 
> Turning this into a UAF doesn't provide any security at all. It
> makes this measurable worse from a security POV as it provides a
> channel for data leakage (read() case) or system unstability or
> compromise (the write() case).

You asked what the concern is, I think you get it, you explained it to
Jan in another email.

We have this issue where if we are not careful we can create a UAF bug
through GUP. It is not something a real application will hit, this is
kernel self-protection against hostile user space trying to trigger a
UAF. The issue arises from both the FS and the MM having their own
lifecycle models for the same memory page.

I'm still not clear on exactly what the current state of affairs is,
Dan?

The DAX/FSDAX stuff currently has a wait on the struct page - does
that wait protect against these UAFs? It looks to me like that is what
it is suppposed to do?

If so, that wait simply needs to be transformed into a wait for the
refcount to be 0 when you rework the refcounting. 

This is not the same FOLL_LONGTERM discussion rehashed, all the
FOLL_LONGTERM discussions were predicated on the idea that GUP
actually worked and doesn't have UAF bugs.

> The *storage media* must not be reused until the filesystem says it
> can be reused. And for that to work, nothing is allowed to keep an
> anonymous, non-filesystem reference to the storage media. It has
> nothing to do with struct page reference counts, and everything to
> do with ensuring that filesystem objects are correctly referenced
> while the storage media is in direct use by an application.

The trouble is we have *two* things that think they own the media -
the mm through pgmap clearly is the owner of the struct page and has
its own well defined lifecycle model for it.

And the FS has its model. We have to ensure the two models are tied
together, a page in the media cannot be considered free until both
lifecycle models agree it is free.

This is a side effect of using the struct pages in the first place,
currently the FS can't use struct page but opt out of the mm's
lifecycle model for struct page!

If we want the FS to own everything exclusively we should purge the
struct pages completely and give up all the features that come with
them (like GUP)

Jason