On 11/03/2024 10:28, Amit Daniel Kachhap wrote:
[...]
+#ifdef CONFIG_CHERI_PURECAP_UABI +#define reserv_representable_alignment(len) \
- (test_bit(MMF_PCUABI_RESERV, ¤t->mm->flags) \
- ? ~cheri_representable_alignment_mask(len) : 0)
The value info.align_mask is set to always seems to be ANDed with PAGE_MASK (see for instance hugetlb_get_unmapped_area_bottomup()), presumably to avoid adding PAGE_SIZE - 1 in the calculations in unmapped_area(). I guess we should do the same here?
+#define reserv_representable_base(base, len) \
- (test_bit(MMF_PCUABI_RESERV, ¤t->mm->flags) \
- ? base & cheri_representable_alignment_mask(len) : base)
Macro arguments should be enclosed in parentheses when they're referenced (also applies to the !PCuABI definitions).
+#define reserv_representable_length(len) \
- (test_bit(MMF_PCUABI_RESERV, ¤t->mm->flags) \
- ? cheri_representable_length(len) : len)
+#define reserv_vma_reserv_start(vma) \
- (test_bit(MMF_PCUABI_RESERV, &vma->vm_mm->flags) \
? vma->reserv_data.reserv_start : 0)+#define reserv_vma_reserv_len(vma) \
- (test_bit(MMF_PCUABI_RESERV, &vma->vm_mm->flags) \
? vma->reserv_data.reserv_len : 0)+#define reserv_vma_reserv_perm(vma) \
Sorry I didn't mention this earlier: "perms" (plural) is generally better throughout the code, a capability almost always has multiple permissions.
- (test_bit(MMF_PCUABI_RESERV, &vma->vm_mm->flags) \
? vma->reserv_data.reserv_perm : 0)[...]
+/**
- reserv_is_supported() - Checks if the reservation property exists for the mm.
- @mm: The mm pointer.
- Return: True if mm has the reservation property set or false otherwise.
- */
+static inline bool reserv_is_supported(struct mm_struct *mm) +{
- if (mm && test_bit(MMF_PCUABI_RESERV, &mm->flags))
Is the null check really warranted for mm? Is there any situation where current->mm or vma->vm_mm (what this function is passed in practice) is null?
return true;- return false;
Or simply:
return mm && test_bit(MMF_PCUABI_RESERV, &mm->flags);
Same comment for a few functions in cap_addr_mgmt.c.
+}
[...] +struct reserv_struct {
- ptraddr_t reserv_start;
- size_t reserv_len;
- user_ptr_perms_t reserv_perm;
Having moved all the fields to their own struct, I guess the reserv_ prefix becomes redundant: simply start, len, perms?
+};
/*
- This struct describes a virtual memory area. There is one of these
- per VM-area/task. A VM area is any part of the process virtual memory
@@ -711,6 +717,9 @@ struct vm_area_struct { struct vma_numab_state *numab_state; /* NUMA Balancing state */ #endif struct vm_userfaultfd_ctx vm_userfaultfd_ctx; +#ifdef CONFIG_CHERI_PURECAP_UABI
- struct reserv_struct reserv_data;
+#endif } __randomize_layout; #ifdef CONFIG_NUMA diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 685586bc0d89..d663c6105d54 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -2,6 +2,7 @@ #ifndef _LINUX_USER_PTR_H #define _LINUX_USER_PTR_H +#include <linux/cheri.h> #include <linux/limits.h> #include <linux/typecheck.h> @@ -27,6 +28,8 @@ #ifdef CONFIG_CHERI_PURECAP_UABI +#define user_ptr_perms_t cheri_perms_t
/**
- uaddr_to_user_ptr() - Convert a user-provided address to a user pointer.
- @addr: The address to set the pointer to.
@@ -109,6 +112,8 @@ bool check_user_ptr_rw(void __user *ptr, size_t len); #else /* CONFIG_CHERI_PURECAP_UABI */ +#define user_ptr_perms_t int
static inline void __user *uaddr_to_user_ptr(ptraddr_t addr) { return as_user_ptr(addr); diff --git a/mm/Makefile b/mm/Makefile index 33873c8aedb3..6f994a1664e4 100644 --- a/mm/Makefile +++ b/mm/Makefile @@ -39,7 +39,7 @@ mmu-y := nommu.o mmu-$(CONFIG_MMU) := highmem.o memory.o mincore.o \ mlock.o mmap.o mmu_gather.o mprotect.o mremap.o \ msync.o page_vma_mapped.o pagewalk.o \
pgtable-generic.o rmap.o vmalloc.o
pgtable-generic.o rmap.o vmalloc.o cap_addr_mgmt.o
We could add the file to mmu-$(CONFIG_CHERI_PURECAP_UABI) instead, this way we don't need the #ifdef in the .c.
[...]
+bool reserv_vma_range_within_reserv(struct vm_area_struct *vma, ptraddr_t start, size_t len) +{
- if (!reserv_is_supported(vma->vm_mm))
return true;- start = untagged_addr(start);
This is strange as we're not doing that in any other function. AFAICT reserv_vma_range_within_reserv() is never passed an address that could be tagged (i.e. directly from the user, without untaged_addr() having been called already).
Kevin
- /* Check if there is match with the existing reservations */
- if (vma->reserv_data.reserv_start <= start && vma->reserv_data.reserv_len >= len)
return true;- return false;
+}
+#endif /* CONFIG_CHERI_PURECAP_UABI */