Commit title: this is quite long (80 char). The ideal target is 50 char, though it's often hard to be that short. Keeping it under 75 char is highly recommended though.
I also forgot to mention on the other patches, it's not a hard rule but in general the arm64 patches that are clearly specific to an extension are tagged as such, so all patches in that series could be "arm64: morello:" (this is the case for all the original Morello enablement patches).
Putting that together, the title for this one could be for instance "arm64: morello: Explicitly add VM_*_CAPS to private user mappings" (the fact that it replaces the hardcoding of PTE_*_CAPS in pgtable-prot.h is probably too much for the commit title).
On 02/11/2022 12:08, Tudor Cretu wrote:
Don't enable PTE_*_CAPS to default user mappings in pgtable-prot.h.
s/enable/add/?
Instead, add VM_{READ,WRITE}_CAPS to all private user mappings. This ensures that the rc/wc smaps flags are set. The instances where the VM_*_CAPS flags are added by default are:
- the stack and brk area by updating the VM_DATA_DEFAULT_FLAGS flags
- private user mappings created by userspace through mmap()
Ah, but what about mappings created by the kernel itself during execve()? I think this is already taken care of in fact as calc_vm_flag_bits() is used by do_mmap(), which is the implementation both of the syscall and vm_mmap() used for instance by binfmt_elf.c. Still, worth clarifying that this also works for standard mappings created by the kernel.
Signed-off-by: Tudor Cretu tudor.cretu@arm.com
arch/arm64/include/asm/mman.h | 13 +++++++++++-- arch/arm64/include/asm/page.h | 2 +- arch/arm64/include/asm/pgtable-prot.h | 12 +++++------- 3 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h index eb0b862121a2..88b0be497231 100644 --- a/arch/arm64/include/asm/mman.h +++ b/arch/arm64/include/asm/mman.h @@ -23,15 +23,24 @@ static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags) {
- unsigned long ret = 0;
- /*
*/ if (system_supports_mte() && (flags & MAP_ANONYMOUS))
- Only allow MTE on anonymous mappings as these are guaranteed to be
- backed by tags-capable memory. The vm_flags may be overridden by a
- filesystem supporting MTE (RAM-based).
return VM_MTE_ALLOWED;
ret |= VM_MTE_ALLOWED;- /*
* Allow capability tag access to private mappings as they don't pose
s/to/for/
Since we are now doing the right thing by adding VM flags to enable tag access, could you add a TODO regarding file-backed mappings? What the MTE comment says above applies pretty much exactly to Morello too, but we really need to have tag access in certain private file-backed mappings so we took a shortcut. Shared file-backed mappings are obviously a problem (what if the backing storage can't hold tags?), but private ones can also be an issue, notably because of DAX (see Documentation/filesystems/dax.rst). In other words, we shouldn't blindly add the VM flags to any file-backed mapping, it should only be done when supported, which might required an ALLOWED flag like for MTE. Anyway, a short TODO here would be nice so that people don't think we missed that :)
* the risk of leaking capabilities outside their original address-space.*/- if (system_supports_morello() && ((flags & MAP_TYPE) == 0x02 /* MAP_PRIVATE */))
Could we potentially include <uapi/linux/mman.h> instead of <uapi/asm/mman.h> to get MAP_PRIVATE, or is the header soup too bad for that?
ret |= VM_READ_CAPS | VM_WRITE_CAPS;
- return 0;
- return ret;
} #define arch_calc_vm_flag_bits(flags) arch_calc_vm_flag_bits(flags) diff --git a/arch/arm64/include/asm/page.h b/arch/arm64/include/asm/page.h index 993a27ea6f54..1c3ed7017d86 100644 --- a/arch/arm64/include/asm/page.h +++ b/arch/arm64/include/asm/page.h @@ -47,7 +47,7 @@ int pfn_is_map_memory(unsigned long pfn); #endif /* !__ASSEMBLY__ */ -#define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED) +#define VM_DATA_DEFAULT_FLAGS (VM_DATA_FLAGS_TSK_EXEC | VM_MTE_ALLOWED | VM_RW_CAPS) #include <asm-generic/getorder.h> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h index 5463f9bc0602..ab746df170e3 100644 --- a/arch/arm64/include/asm/pgtable-prot.h +++ b/arch/arm64/include/asm/pgtable-prot.h @@ -90,11 +90,9 @@ extern bool arm64_use_ng_mappings; #define PAGE_NONE __pgprot(((_USER_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_RDONLY | PTE_UXN) /* shared+writable pages are clean by default, hence PTE_RDONLY|PTE_WRITE */ #define PAGE_SHARED __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_UXN | PTE_WRITE) -#define PAGE_SHARED_RO __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_UXN) -#define PAGE_SHARED_RO_EXEC __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY) #define PAGE_SHARED_EXEC __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_WRITE) -#define PAGE_READONLY __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_MAYBE_LS_CAPS | PTE_UXN) -#define PAGE_READONLY_EXEC __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_MAYBE_LS_CAPS) +#define PAGE_READONLY __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY | PTE_UXN) +#define PAGE_READONLY_EXEC __pgprot(_USER_PAGE_DEFAULT | PTE_RDONLY) #define PAGE_EXECONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN) #define __P000 PAGE_NONE @@ -107,11 +105,11 @@ extern bool arm64_use_ng_mappings; #define __P111 PAGE_READONLY_EXEC #define __S000 PAGE_NONE -#define __S001 PAGE_SHARED_RO +#define __S001 PAGE_READONLY #define __S010 PAGE_SHARED #define __S011 PAGE_SHARED -#define __S100 PAGE_SHARED_RO_EXEC /* PAGE_EXECONLY if Enhanced PAN */ -#define __S101 PAGE_SHARED_RO_EXEC +#define __S100 PAGE_READONLY_EXEC /* PAGE_EXECONLY if Enhanced PAN */ +#define __S101 PAGE_READONLY_EXEC #define __S110 PAGE_SHARED_EXEC #define __S111 PAGE_SHARED_EXEC
I think it would be nice to mention in the commit message that this is effectively a partial revert of "arm64: morello: Enable access to capabilities in memory" (reverting the addition of the PTE flags to the private user mappings).
Kevin