Re: [PATCH v3] mm/page_table_check: Support userfault wr-protect entries

From: Pasha Tatashin
Date: Wed Apr 17 2024 - 15:45:19 EST


On Wed, Apr 17, 2024 at 3:26 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> Allow page_table_check hooks to check over userfaultfd wr-protect criteria
> upon pgtable updates. The rule is no co-existance allowed for any writable
> flag against userfault wr-protect flag.
>
> This should be better than c2da319c2e, where we used to only sanitize such
> issues during a pgtable walk, but when hitting such issue we don't have a
> good chance to know where does that writable bit came from [1], so that
> even the pgtable walk exposes a kernel bug (which is still helpful on
> triaging) but not easy to track and debug.
>
> Now we switch to track the source. It's much easier too with the recent
> introduction of page table check.
>
> There are some limitations with using the page table check here for
> userfaultfd wr-protect purpose:
>
> - It is only enabled with explicit enablement of page table check configs
> and/or boot parameters, but should be good enough to track at least
> syzbot issues, as syzbot should enable PAGE_TABLE_CHECK[_ENFORCED] for
> x86 [1]. We used to have DEBUG_VM but it's now off for most distros,
> while distros also normally not enable PAGE_TABLE_CHECK[_ENFORCED], which
> is similar.
>
> - It conditionally works with the ptep_modify_prot API. It will be
> bypassed when e.g. XEN PV is enabled, however still work for most of the
> rest scenarios, which should be the common cases so should be good
> enough.
>
> - Hugetlb check is a bit hairy, as the page table check cannot identify
> hugetlb pte or normal pte via trapping at set_pte_at(), because of the
> current design where hugetlb maps every layers to pte_t... For example,
> the default set_huge_pte_at() can invoke set_pte_at() directly and lose
> the hugetlb context, treating it the same as a normal pte_t. So far it's
> fine because we have huge_pte_uffd_wp() always equals to pte_uffd_wp() as
> long as supported (x86 only). It'll be a bigger problem when we'll
> define _PAGE_UFFD_WP differently at various pgtable levels, because then
> one huge_pte_uffd_wp() per-arch will stop making sense first.. as of now
> we can leave this for later too.
>
> This patch also removes commit c2da319c2e altogether, as we have something
> better now.
>
> [1] https://lore.kernel.org/all/000000000000dce0530615c89210@xxxxxxxxxx/
>
> Cc: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
> v2:
> - Rename __page_table_check_pxx() to page_table_check_pxx_flags(),
> meanwhile move the pte check out of the loop [Pasha]
> - Fix build issues reported from the bot, also added SWP_DEVICE_WRITE which
> was overlooked before
> v3:
> - Add missing doc update [Pasha]
> ---
> Documentation/mm/page_table_check.rst | 9 ++++++-
> arch/x86/include/asm/pgtable.h | 18 +------------
> mm/page_table_check.c | 39 +++++++++++++++++++++++++++
> 3 files changed, 48 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/mm/page_table_check.rst b/Documentation/mm/page_table_check.rst
> index c12838ce6b8d..5bd1d987d76d 100644
> --- a/Documentation/mm/page_table_check.rst
> +++ b/Documentation/mm/page_table_check.rst
> @@ -14,7 +14,7 @@ Page table check performs extra verifications at the time when new pages become
> accessible from the userspace by getting their page table entries (PTEs PMDs
> etc.) added into the table.
>
> -In case of detected corruption, the kernel is crashed. There is a small
> +In case of most detected corruption, the kernel is crashed. There is a small
> performance and memory overhead associated with the page table check. Therefore,
> it is disabled by default, but can be optionally enabled on systems where the
> extra hardening outweighs the performance costs. Also, because page table check
> @@ -22,6 +22,13 @@ is synchronous, it can help with debugging double map memory corruption issues,
> by crashing kernel at the time wrong mapping occurs instead of later which is
> often the case with memory corruptions bugs.
>
> +It can also be used to do page table entry checks over various flags, dump
> +warnings when illegal combinations of entry flags are detected. Currently,
> +userfaultfd is the only user of such to sanity check wr-protect bit against
> +any writable flags. Illegal flag combinations will not directly cause data
> +corruption in this case immediately, but that will cause read-only data to
> +be writable, causing data corrupt when the page content is later modified.

I would replace: "causing data corrupt ..." to "leading to corruption ..."

> +
> Double mapping detection logic
> ==============================
>
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 273f7557218c..65b8e5bb902c 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -388,23 +388,7 @@ static inline pte_t pte_wrprotect(pte_t pte)
> #ifdef CONFIG_HAVE_ARCH_USERFAULTFD_WP
> static inline int pte_uffd_wp(pte_t pte)
> {
> - bool wp = pte_flags(pte) & _PAGE_UFFD_WP;
> -
> -#ifdef CONFIG_DEBUG_VM
> - /*
> - * Having write bit for wr-protect-marked present ptes is fatal,
> - * because it means the uffd-wp bit will be ignored and write will
> - * just go through.
> - *
> - * Use any chance of pgtable walking to verify this (e.g., when
> - * page swapped out or being migrated for all purposes). It means
> - * something is already wrong. Tell the admin even before the
> - * process crashes. We also nail it with wrong pgtable setup.
> - */
> - WARN_ON_ONCE(wp && pte_write(pte));
> -#endif
> -
> - return wp;
> + return pte_flags(pte) & _PAGE_UFFD_WP;
> }
>
> static inline pte_t pte_mkuffd_wp(pte_t pte)
> diff --git a/mm/page_table_check.c b/mm/page_table_check.c
> index af69c3c8f7c2..388bcf60d8b5 100644
> --- a/mm/page_table_check.c
> +++ b/mm/page_table_check.c
> @@ -7,6 +7,8 @@
> #include <linux/kstrtox.h>
> #include <linux/mm.h>
> #include <linux/page_table_check.h>
> +#include <linux/swap.h>
> +#include <linux/swapops.h>
>
> #undef pr_fmt
> #define pr_fmt(fmt) "page_table_check: " fmt
> @@ -182,6 +184,31 @@ void __page_table_check_pud_clear(struct mm_struct *mm, pud_t pud)
> }
> EXPORT_SYMBOL(__page_table_check_pud_clear);
>
> +/* Whether the swap entry cached writable information */
> +static inline bool swap_cached_writable(swp_entry_t entry)
> +{
> + unsigned type = swp_type(entry);
> +
> +#ifdef CONFIG_DEVICE_PRIVATE
> + if (type == SWP_DEVICE_EXCLUSIVE_WRITE || type == SWP_DEVICE_WRITE)
> + return true;
> +#endif
> +#ifdef CONFIG_MIGRATION
> + if (type == SWP_MIGRATION_WRITE)
> + return true;
> +#endif
> +
> + return false;
> +}

This should be re-written like this:

static inline bool swap_cached_writable(swp_entry_t entry)
{
return is_writable_device_exclusive_entry(entry) ||
is_writable_device_private_entry(entry) ||
is_writable_migration_entry(entry);
}

Otherwise the patch looks good.

Pasha