On 17/07/2023 07:11, Chaitanya S Prakash wrote:
As mremap() doesn't take prot flags into consideration, the permissions
Better remove "into consideration" - mremap() simply doesn't take a prot argument (otherwise it sounds like it takes one and ignores it).
of the original address are retained. If the permissions of the new
"address" -> "pointer" (or maybe, more to the point, capability)
We need to be quite precise on the pointer/address terminology, otherwise things get confusing pretty quickly. The fact that the arguments are "officially" called old_addr / new_addr is certainly not helping us in that respect...
address do not match the set of permissions of the old address, the syscall fails with a -EINVAL error code. A test to verify this behaviour has been added.
Signed-off-by: Chaitanya S Prakash chaitanyas.prakash@arm.com
tools/testing/selftests/arm64/morello/mmap.c | 68 ++++++++++++++++++++ 1 file changed, 68 insertions(+)
diff --git a/tools/testing/selftests/arm64/morello/mmap.c b/tools/testing/selftests/arm64/morello/mmap.c index aa552c8affba..3b8e99aafedc 100644 --- a/tools/testing/selftests/arm64/morello/mmap.c +++ b/tools/testing/selftests/arm64/morello/mmap.c @@ -165,6 +165,68 @@ static void purecap_map_growsdown(void) ASSERT_EQ((unsigned long)addr, (unsigned long)-EOPNOTSUPP); } +/* test to verify mremap() behaviour when permissions are modified */ +static void purecap_mremap_perms_check(void) +{
- void *old_addr, *new_addr, *ret;
- int retval;
- int prot = PROT_READ | PROT_WRITE;
- int flags = MAP_PRIVATE | MAP_ANONYMOUS;
- size_t old_perms, new_perms;
- /* permissions of capability returned by mremap must match the
* permissions returned by the original mapping */- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, prot, flags, -1, 0);
- ASSERT_GT((unsigned long)old_addr, 0);
- old_perms = cheri_perms_get(old_addr);
- new_addr = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE, 0);- ASSERT_GT((unsigned long)new_addr, 0);
- new_perms = cheri_perms_get(new_addr);
- ASSERT_EQ(old_perms, new_perms);
- EXPECT_EQ(0, probe_mem_range(new_addr, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* remapping to a new_addr having reduced permissions from old_addr */
- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot | PROT_EXEC) |
prot | PROT_EXEC, flags, -1, 0);- ASSERT_GT((unsigned long)old_addr, 0);
- new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot) | prot, flags, -1, 0);
As a rule, PROT_MAX(prot) | prot is exactly equivalent to just prot.
- ASSERT_GT((unsigned long)new_addr, 0);
- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(0, retval);
- ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE | MREMAP_FIXED, new_addr);
ret is unchecked here, and that's unfortunate because this mremap() will fail: either the reservation at new_addr is destroyed and new_addr becomes an invalid owning capability, or the reservation has been destroyed yet - also invalid as it is forbidden to reuse unmapped space in a reservation.
The safe thing we can do is to overwrite the mapping directly - it should be as simple as removing the munmap() above.
- EXPECT_EQ(0, probe_mem_range(ret, MMAP_SIZE,
PROBE_MODE_TOUCH | PROBE_MODE_VERIFY));- retval = munmap(ret, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- /* remapping to new_addr having increased permissions from old_addr */
- old_addr = mmap(NULL, MMAP_SIZE_REDUCED, PROT_MAX(prot) | prot, flags,
-1, 0);- ASSERT_GT((unsigned long)old_addr, 0);
- new_addr = mmap(NULL, MMAP_SIZE, PROT_MAX(prot | PROT_EXEC) | prot |
PROT_EXEC, flags, -1, 0);- ASSERT_GT((unsigned long)new_addr, 0);
- retval = munmap(new_addr, MMAP_SIZE);
- ASSERT_EQ(retval, 0);
- ret = mremap(old_addr, MMAP_SIZE_REDUCED, MMAP_SIZE,
MREMAP_MAYMOVE | MREMAP_FIXED, new_addr);
This would fail anyway because the mapping at new_addr has already been removed (same rationale as above). We should try to avoid relying on the prioritisation of error codes, so let's move the munmap() after this mremap().
Kevin
- ASSERT_EQ((unsigned long)ret, (unsigned long)-EINVAL);