Re: [PATCH V8 32/44] memremap_pages: Add Kconfig for DEVMAP_ACCESS_PROTECTION

[Dan Williams <dan.j.williams@intel.com>]

On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote:
>
> From: Ira Weiny <ira.weiny@intel.com>
>
> The persistent memory (PMEM) driver uses the memremap_pages facility to
> provide 'struct page' metadata (vmemmap) for PMEM.  Given that PMEM
> capacity maybe orders of magnitude higher capacity than System RAM it

s/maybe/may be/

> presents a large vulnerability surface to stray writes.  Unlike stray
> writes to System RAM, which may result in a crash or other undesirable
> behavior, stray writes to PMEM additionally are more likely to result in
> permanent data loss. Reboot is not a remediation for PMEM corruption
> like it is for System RAM.
>
> Given that PMEM access from the kernel is limited to a constrained set
> of locations (PMEM driver, Filesystem-DAX, and direct-I/O to a DAX
> page), it is amenable to supervisor pkey protection.
>
> Not all systems with PMEM will want additional protections.  Therefore,
> add a Kconfig option for the user to configure the additional devmap
> protections.
>
> Only systems with supervisor protection keys (PKS) are able to support
> this new protection so depend on ARCH_HAS_SUPERVISOR_PKEYS.
> Furthermore, select ARCH_ENABLE_SUPERVISOR_PKEYS to ensure that the
> architecture support is enabled if PMEM is the only use case.
>
> Only PMEM which is advertised to the memory subsystem needs this
> protection.  Therefore, the feature depends on NVDIMM_PFN.
>
> A default of (NVDIMM_PFN && ARCH_HAS_SUPERVISOR_PKEYS) was suggested but
> logically that is the same as saying default 'yes' because both
> NVDIMM_PFN and ARCH_HAS_SUPERVISOR_PKEYS are required.  Therefore a
> default of 'yes' is used.

It still feels odd to default this to y just because the ARCH enables
it. I think it's fine for this to require explicit opt-in especially
because it has non-zero overhead and there are other PKEYS users on
the horizon.

>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> ---
> Changes for V8
>         Split this out from
>                 [PATCH V7 13/18] memremap_pages: Add access protection via supervisor Protection Keys (PKS)
> ---
>  mm/Kconfig | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 46f2bb15aa4e..67e0264acf7d 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -776,6 +776,24 @@ config ZONE_DEVICE
>
>           If FS_DAX is enabled, then say Y.
>
> +config DEVMAP_ACCESS_PROTECTION
> +       bool "Access protection for memremap_pages()"
> +       depends on NVDIMM_PFN
> +       depends on ARCH_HAS_SUPERVISOR_PKEYS
> +       select ARCH_ENABLE_SUPERVISOR_PKEYS
> +       default y
> +
> +       help
> +         Enable extra protections on device memory.  This protects against
> +         unintended access to devices such as a stray writes.  This feature is
> +         particularly useful to protect against corruption of persistent
> +         memory.
> +
> +         This depends on architecture support of supervisor PKeys and has no
> +         overhead if the architecture does not support them.
> +
> +         If you have persistent memory say 'Y'.
> +
>  config DEV_PAGEMAP_OPS
>         bool
>
> --
> 2.31.1
>