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

Dan Williams
2022-02-04T15:49:40Z

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
>