On 7/20/23 13:13, Kevin Brodsky wrote:
On 17/07/2023 07:11, Chaitanya S Prakash wrote:
Only valid owning capabilities have the ability to modify an address space by making use of the appropriate syscalls, any deviation from this method must result in immediate failure of the syscall. When a capability is created it is assigned the maximum permissions it may ever have by passing PROT_MAX(max_prot) as one of the prot flags. Any attempt to increase the permissions beyond this would result in failure of the syscall.
A test to validate the parmeters passed to address space management syscalls has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 76 ++++++++++++++++++++ 1 file changed, 76 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index 00d4c5ea9703..184dcf4ddc92 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -128,6 +128,76 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); }
+/* test to validate parameters passed to address space management syscalls */ +static inline void purecap_param_check(void) +{
- void *addr, *addr_cap, *null_cap_addr;
- ptraddr_t address;
- int retval;
- int max_prot = PROT_READ | PROT_WRITE | PROT_EXEC;
- int prot = max_prot;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- /* generate null-derived capability to be used later*/
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- address = cheri_address_get(addr);
- null_cap_addr = (void *)(uintptr_t)address;
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* passing invalid capability to sycall */
- addr = mmap(NULL, MMAP_SIZE, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- addr_cap = cheri_tag_clear(addr);
addr_cap is a strange name, invalid_cap maybe? Alternatively you don't really need a temporary at all here, you could directly pass cheri_tag_clear(addr) to munmap().
I'll make the change.
- retval = munmap(addr_cap, MMAP_SIZE);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* increase permission beyond the maximum prot specified for the mapping */
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(PROT_READ | PROT_WRITE) |
PROT_READ | PROT_WRITE, flags, -1, 0);Passing R|W as prot already implies PROT_MAX(R|W), so there is no need to use PROT_MAX() for that test.
Got it, I'll change it.
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_EXEC);
- ASSERT_EQ(retval, -EINVAL);
- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* max_prot has fewer permissions than prot */
- max_prot = (prot & ~(PROT_READ));
Reusing the value of prot and/or prot_max set by previous cases (or at the beginning) is not very readable. I would rather have every case set prot and prot_max explicitly. Feel free to define a macro set to R|W|X to save some typing.
Sure, I'll make that change.
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_EQ((unsigned long)addr, (unsigned long)-EINVAL);
- /* max_prot has more permissions than prot */
- max_prot = prot;
- prot = (prot & ~(PROT_WRITE));
- addr = mmap(NULL, MMAP_SIZE, PROT_MAX(max_prot) | prot, flags, -1, 0);
- ASSERT_GT((unsigned long)addr, 0);
- retval = mprotect(addr, MMAP_SIZE, PROT_WRITE);
- ASSERT_NE(retval, -EINVAL);
Why wouldn't we check that the call actually succeeded? I think I've noticed this pattern in other patches too. In general it is not enough to check that the return value is not a particular error, we need to check that it is valid (or a particular error).
I must have changed it during testing and missed it, I'll go though it and change it accordingly.
- EXPECT_EQ(0, probe_mem_range(addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* repeat positive max_prot test with null-derived capability */
- addr = mmap(null_cap_addr, MMAP_SIZE, PROT_MAX(max_prot) | prot,
flags | MAP_FIXED, -1, 0);We should avoid relying on this address that has been previously obtained from mmap() being available. That's because it is unspecified when reservations are destroyed (see the "Lifetime of reservations" note in the spec), meaning that the reservation for that initial mmap() may still exist. The MAP_FIXED + address use-case is pretty limited, but I suppose it's still worth testing. What we could do is use a hardcoded address that is not going to be ever mapped. That's not so easy to come by, but I think the very beginning of the address space is quite safe (not 0x0 though, which cannot be mapped). Maybe at address == page_size?
I see what you mean, I'll change it in this as well as patch 5.
Kevin