Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 12 May 2023 00:07:59 +0800
From:   Ruihan Li <lrh2000@pku.edu.cn>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-mm@kvack.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        stable@vger.kernel.org, Ruihan Li <lrh2000@pku.edu.cn>
Subject: Re: [PATCH 3/4] mm: page_table_check: Make it dependent on !DEVMEM
Message-ID: <ehnawbpypxg5vppwcoiuswlq5c54td5mos4jldf5tuy7wbjlvb@o7fydrxd2y34>
References: <20230510085527.57953-1-lrh2000@pku.edu.cn>
 <20230510085527.57953-4-lrh2000@pku.edu.cn>
 <f74f2080-1def-f9c2-8884-97bb4c8ba4d1@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f74f2080-1def-f9c2-8884-97bb4c8ba4d1@redhat.com>
Precedence: bulk

On Wed, May 10, 2023 at 06:40:31PM +0200, David Hildenbrand wrote:
> On 10.05.23 10:55, Ruihan Li wrote:
> > The special device /dev/mem enables users to map arbitrary physical
> > memory regions into the user space, which can conflict with the double
> > mapping detection logic used by the page table check. For instance,
> > pages may change their properties (e.g., from anonymous pages to named
> > pages) while they are still being mapped in the user space via /dev/mem,
> > leading to "corruption" detected by the page table check.
> > 
> > To address this issue, the PAGE_TABLE_CHECK config option is now
> > dependent on !DEVMM. This ensures that the page table check cannot be
> > enabled when /dev/mem is used. It should be noted that /dev/mem itself
> > is a significant security issue, and its conflict with a hardening
> > technique is understandable.
> > 
> > Cc: <stable@vger.kernel.org> # 5.17
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> >   Documentation/mm/page_table_check.rst | 18 ++++++++++++++++++
> >   mm/Kconfig.debug                      |  2 +-
> >   2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
> > index cfd8f4117..b04f29230 100644
> > --- a/Documentation/mm/page_table_check.rst
> > +++ b/Documentation/mm/page_table_check.rst
> > @@ -52,3 +52,21 @@ Build kernel with:
> >   Optionally, build kernel with PAGE_TABLE_CHECK_ENFORCED in order to have page
> >   table support without extra kernel parameter.
> > +
> > +Implementation notes
> > +====================
> > +
> > +We specifically decided not to use VMA information in order to avoid relying on
> > +MM states (except for limited "struct page" info). The page table check is a
> > +separate from Linux-MM state machine that verifies that the user accessible
> > +pages are not falsely shared.
> > +
> > +As a result, special devices that violate the model cannot live with
> > +PAGE_TABLE_CHECK. Currently, /dev/mem is the only known example. Given it
> > +allows users to map arbitrary physical memory regions into the userspace, any
> > +pages may change their properties (e.g., from anonymous pages to named pages)
> > +while they are still being mapped in the userspace via /dev/mem, leading to
> > +"corruption" detected by the page table check. Therefore, the PAGE_TABLE_CHECK
> > +config option is now dependent on !DEVMEM. It's worth noting that /dev/mem
> > +itself is a significant security issue, and its conflict with a hardening
> > +technique is understandable.
> > diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
> > index a925415b4..37f3d5b20 100644
> > --- a/mm/Kconfig.debug
> > +++ b/mm/Kconfig.debug
> > @@ -97,7 +97,7 @@ config PAGE_OWNER
> >   config PAGE_TABLE_CHECK
> >   	bool "Check for invalid mappings in user page tables"
> > -	depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK
> > +	depends on ARCH_SUPPORTS_PAGE_TABLE_CHECK && !DEVMEM
> >   	select PAGE_EXTENSION
> >   	help
> >   	  Check that anonymous page is not being mapped twice with read write
> 
> That might disable it in a lot of environments I'm afraid. I wonder if we
> could allow it for STRICT_DEVMEM. Hm ...
> -- 
> Thanks,
> 
> David / dhildenb

That sounds pretty reasonable. However, I'm not quite sure if PageAnon
makes sense of (and is guaranteed to work well with) I/O memory pages,
which should be the only pages allowed to be accessed via /dev/mem under
STRICT_DEVMEM.

A quick test has shown that PageAnon (by accident or design?) results in
"false" for I/O memory pages. Meanwhile, the logic used in the page
table check allows named (i.e., non-anonymous) pages to be shared
arbitrarily (i.e. in both read-only and read-write modes) between
processes. So it looks that everything works fine. But is it a
coincidence?

Thanks,
Ruihan Li