diff mbox series

[3/4] mm: page_table_check: Make it dependent on !DEVMEM

Message ID 20230510085527.57953-4-lrh2000@pku.edu.cn
State New
Headers show
Series Fix type confusion in page_table_check | expand

Commit Message

Ruihan Li May 10, 2023, 8:55 a.m. UTC
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(-)

Comments

David Hildenbrand May 10, 2023, 4:40 p.m. UTC | #1
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 ...
Ruihan Li May 11, 2023, 4:07 p.m. UTC | #2
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
diff mbox series

Patch

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