From: Jeff Xu jeffxu@chromium.org
This series increase the test coverage of mseal_test by:
Add check for vma_size, prot, and error code for existing tests. Add more testcases for madvise, munmap, mmap and mremap to cover sealing in different scenarios.
The increase test coverage hopefully help to prevent future regression. It doesn't change any existing mm api's semantics, i.e. it will pass on linux main and 6.10 branch.
Note: in order to pass this test in mm-unstable, mm-unstable must have Liam's fix on mmap [1]
[1] https://lore.kernel.org/linux-kselftest/vyllxuh5xbqmaoyl2mselebij5ox7cseekjc...
History: V3: - no-functional change, incooperate feedback from Pedro Falcato
V2: - https://lore.kernel.org/linux-kselftest/20240829214352.963001-1-jeffxu@chrom... - remove the mmap fix (Liam R. Howlett will fix it separately) - Add cover letter (Lorenzo Stoakes) - split the testcase for ease of review (Mark Brown)
V1: - https://lore.kernel.org/linux-kselftest/20240828225522.684774-1-jeffxu@chrom...
Jeff Xu (5): selftests/mseal_test: Check vma_size, prot, error code. selftests/mseal: add sealed madvise type selftests/mseal: munmap across multiple vma ranges. selftests/mseal: add more tests for mmap selftests/mseal: add more tests for mremap
tools/testing/selftests/mm/mseal_test.c | 830 ++++++++++++++++++++++-- 1 file changed, 763 insertions(+), 67 deletions(-)
From: Jeff Xu jeffxu@chromium.org
Add more checks for vma size, prot bits and api errcode after attempt of modifing sealed mappings.
Signed-off-by: Jeff Xu jeffxu@chromium.org --- tools/testing/selftests/mm/mseal_test.c | 398 ++++++++++++++++++++---- 1 file changed, 332 insertions(+), 66 deletions(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e7991e5fdcf3..7198f2314f9b 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -170,18 +170,31 @@ static void set_pkey(int pkey, unsigned long pkey_value) static void setup_single_address(int size, void **ptrOut) { void *ptr; + unsigned long page_size = getpagesize();
- ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); - *ptrOut = ptr; + *ptrOut = (void *) MAP_FAILED; + ptr = mmap(NULL, size + 2 * page_size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + if (ptr != (void *) -1) { + /* add 2 page at the beginning and end to avoid auto-merge of mapping */ + sys_mprotect(ptr, page_size, PROT_NONE); + sys_mprotect(ptr + size + page_size, page_size, PROT_NONE); + *ptrOut = ptr + page_size; + } }
static void setup_single_address_rw(int size, void **ptrOut) { void *ptr; unsigned long mapflags = MAP_ANONYMOUS | MAP_PRIVATE; + unsigned long page_size = getpagesize();
- ptr = mmap(NULL, size, PROT_READ | PROT_WRITE, mapflags, -1, 0); - *ptrOut = ptr; + *ptrOut = (void *) MAP_FAILED; + ptr = mmap(NULL, size + 2 * page_size, PROT_READ | PROT_WRITE, mapflags, -1, 0); + if (ptr != (void *) -1) { + sys_mprotect(ptr, page_size, PROT_NONE); + sys_mprotect(ptr + size + page_size, page_size, PROT_NONE); + *ptrOut = ptr + page_size; + } }
static int clean_single_address(void *ptr, int size) @@ -226,6 +239,21 @@ bool pkey_supported(void) return false; }
+bool get_vma_size_supported(void) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 4 * page_size; + int prot; + + setup_single_address(size, &ptr); + size = get_vma_size(ptr, &prot); + if (size == 4 * page_size && prot == 0x4) + return true; + + return false; +} + static void test_seal_addseal(void) { int ret; @@ -419,11 +447,17 @@ static void test_seal_invalid_input(void) unsigned long size = 4 * page_size; int ret;
- setup_single_address(8 * page_size, &ptr); + setup_single_address(9 * page_size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); - ret = clean_single_address(ptr + 4 * page_size, 4 * page_size); + + ret = clean_single_address(ptr, page_size); FAIL_TEST_IF_FALSE(!ret);
+ ret = clean_single_address(ptr + 5 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + ptr = ptr + page_size; + /* invalid flag */ ret = syscall(__NR_mseal, ptr, size, 0x20); FAIL_TEST_IF_FALSE(ret < 0); @@ -523,6 +557,7 @@ static void test_seal_mprotect(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -533,9 +568,14 @@ static void test_seal_mprotect(bool seal) }
ret = sys_mprotect(ptr, size, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -547,6 +587,7 @@ static void test_seal_start_mprotect(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -558,9 +599,14 @@ static void test_seal_start_mprotect(bool seal)
/* the first page is sealed. */ ret = sys_mprotect(ptr, page_size, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
/* pages after the first page is not sealed. */ @@ -577,6 +623,7 @@ static void test_seal_end_mprotect(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -593,9 +640,14 @@ static void test_seal_end_mprotect(bool seal) /* last 3 page are sealed */ ret = sys_mprotect(ptr + page_size, page_size * 3, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -607,6 +659,7 @@ static void test_seal_mprotect_unalign_len(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -618,9 +671,14 @@ static void test_seal_mprotect_unalign_len(bool seal)
/* 2 pages are sealed. */ ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
ret = sys_mprotect(ptr + page_size * 2, page_size, @@ -636,6 +694,7 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -646,9 +705,14 @@ static void test_seal_mprotect_unalign_len_variant_2(bool seal)
/* 3 pages are sealed. */ ret = sys_mprotect(ptr, page_size * 3, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
ret = sys_mprotect(ptr + page_size * 3, page_size, @@ -664,6 +728,7 @@ static void test_seal_mprotect_two_vma(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -678,16 +743,26 @@ static void test_seal_mprotect_two_vma(bool seal) }
ret = sys_mprotect(ptr, page_size * 2, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + } else FAIL_TEST_IF_FALSE(!ret);
ret = sys_mprotect(ptr + page_size * 2, page_size * 2, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr + page_size * 2, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -699,6 +774,7 @@ static void test_seal_mprotect_two_vma_with_split(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -719,17 +795,27 @@ static void test_seal_mprotect_two_vma_with_split(bool seal)
/* the second page is sealed. */ ret = sys_mprotect(ptr + page_size, page_size, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + } else FAIL_TEST_IF_FALSE(!ret);
/* the third page is sealed. */ ret = sys_mprotect(ptr + 2 * page_size, page_size, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr + 2 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
/* the fouth page is not sealed. */ @@ -746,6 +832,7 @@ static void test_seal_mprotect_partial_mprotect(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -758,9 +845,14 @@ static void test_seal_mprotect_partial_mprotect(bool seal)
/* mprotect first 2 page will fail, since the first page are sealed. */ ret = sys_mprotect(ptr, 2 * page_size, PROT_READ | PROT_WRITE); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -783,15 +875,15 @@ static void test_seal_mprotect_partial_mprotect_tail(bool seal) }
ret = sys_mprotect(ptr, size, PROT_EXEC); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else - FAIL_TEST_IF_FALSE(!ret); + FAIL_TEST_IF_FALSE(errno == EPERM);
- if (seal) { - FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0); + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); FAIL_TEST_IF_FALSE(prot == 0x4); - } + } else + FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); } @@ -846,6 +938,7 @@ static void test_seal_mprotect_split(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -862,16 +955,34 @@ static void test_seal_mprotect_split(bool seal)
/* mprotect is sealed. */ ret = sys_mprotect(ptr, 2 * page_size, PROT_READ); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
ret = sys_mprotect(ptr + 2 * page_size, 2 * page_size, PROT_READ); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -883,6 +994,7 @@ static void test_seal_mprotect_merge(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -899,9 +1011,18 @@ static void test_seal_mprotect_merge(bool seal)
/* 2 pages are sealed. */ ret = sys_mprotect(ptr, 2 * page_size, PROT_READ); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
/* last 2 pages are not sealed. */ @@ -917,6 +1038,7 @@ static void test_seal_munmap(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -928,9 +1050,14 @@ static void test_seal_munmap(bool seal)
/* 4 pages are sealed. */ ret = sys_munmap(ptr, size); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -948,6 +1075,7 @@ static void test_seal_munmap_two_vma(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -962,15 +1090,33 @@ static void test_seal_munmap_two_vma(bool seal) }
ret = sys_munmap(ptr, page_size * 2); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + + size = get_vma_size(ptr + 2 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
ret = sys_munmap(ptr + page_size, page_size * 2); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x6); + + size = get_vma_size(ptr + 2 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -1018,33 +1164,75 @@ static void test_seal_munmap_partial_across_vmas(bool seal) { void *ptr; unsigned long page_size = getpagesize(); - unsigned long size = 2 * page_size; + unsigned long size = 12 * page_size; int ret; int prot;
- /* - * Check if a partial mseal (that results in two vmas) works correctly. - * It might unmap the first, but it'll never unmap the second (msealed) vma. - */ - setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1);
if (seal) { - ret = sys_mseal(ptr + page_size, page_size); + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); FAIL_TEST_IF_FALSE(!ret); }
- ret = sys_munmap(ptr, size); - if (seal) + ret = sys_munmap(ptr, 12 * page_size); + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 8 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
+ + ret = sys_munmap(ptr, 6 * page_size); if (seal) { - FAIL_TEST_IF_FALSE(get_vma_size(ptr + page_size, &prot) > 0); + FAIL_TEST_IF_FALSE(ret < 0); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); FAIL_TEST_IF_FALSE(prot == 0x4); - } + + size = get_vma_size(ptr + 8 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(!ret); + + ret = sys_munmap(ptr + 6 * page_size, 6 * page_size); + if (seal) { + FAIL_TEST_IF_FALSE(ret < 0); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 8 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); } @@ -1074,9 +1262,11 @@ static void test_munmap_start_freed(bool seal) ret = sys_munmap(ptr, size); if (seal) { FAIL_TEST_IF_FALSE(ret < 0); + FAIL_TEST_IF_FALSE(errno == EPERM);
size = get_vma_size(ptr + page_size, &prot); - FAIL_TEST_IF_FALSE(size == page_size * 3); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { /* note: this will be OK, even the first page is */ /* already unmapped. */ @@ -1095,6 +1285,7 @@ static void test_munmap_end_freed(bool seal) unsigned long page_size = getpagesize(); unsigned long size = 4 * page_size; int ret; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1111,9 +1302,14 @@ static void test_munmap_end_freed(bool seal)
/* unmap all pages. */ ret = sys_munmap(ptr, size); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(!ret);
REPORT_TEST_PASS(); @@ -1144,12 +1340,15 @@ static void test_munmap_middle_freed(bool seal) ret = sys_munmap(ptr, size); if (seal) { FAIL_TEST_IF_FALSE(ret < 0); + FAIL_TEST_IF_FALSE(errno == EPERM);
size = get_vma_size(ptr, &prot); FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4);
size = get_vma_size(ptr + page_size * 3, &prot); FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { FAIL_TEST_IF_FALSE(!ret);
@@ -1170,6 +1369,7 @@ static void test_seal_mremap_shrink(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1184,6 +1384,10 @@ static void test_seal_mremap_shrink(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { FAIL_TEST_IF_FALSE(ret2 != (void *) MAP_FAILED);
@@ -1199,6 +1403,7 @@ static void test_seal_mremap_expand(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1216,6 +1421,10 @@ static void test_seal_mremap_expand(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { FAIL_TEST_IF_FALSE(ret2 == ptr);
@@ -1231,6 +1440,7 @@ static void test_seal_mremap_move(bool seal) unsigned long size = page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1249,10 +1459,12 @@ static void test_seal_mremap_move(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); - } else { - FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
- } + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED);
REPORT_TEST_PASS(); } @@ -1264,6 +1476,7 @@ static void test_seal_mmap_overwrite_prot(bool seal) unsigned long size = page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1279,6 +1492,10 @@ static void test_seal_mmap_overwrite_prot(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == ptr);
@@ -1292,6 +1509,7 @@ static void test_seal_mmap_expand(bool seal) unsigned long size = 12 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1310,6 +1528,10 @@ static void test_seal_mmap_expand(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 8 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == ptr);
@@ -1323,6 +1545,7 @@ static void test_seal_mmap_shrink(bool seal) unsigned long size = 12 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1338,6 +1561,10 @@ static void test_seal_mmap_shrink(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 12 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == ptr);
@@ -1352,6 +1579,7 @@ static void test_seal_mremap_shrink_fixed(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1369,6 +1597,10 @@ static void test_seal_mremap_shrink_fixed(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == newAddr);
@@ -1383,6 +1615,7 @@ static void test_seal_mremap_expand_fixed(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(page_size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1400,6 +1633,10 @@ static void test_seal_mremap_expand_fixed(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(newAddr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == newAddr);
@@ -1414,6 +1651,7 @@ static void test_seal_mremap_move_fixed(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1430,6 +1668,10 @@ static void test_seal_mremap_move_fixed(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(newAddr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else FAIL_TEST_IF_FALSE(ret2 == newAddr);
@@ -1443,6 +1685,7 @@ static void test_seal_mremap_move_fixed_zero(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1460,9 +1703,12 @@ static void test_seal_mremap_move_fixed_zero(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); - } else { + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else FAIL_TEST_IF_FALSE(ret2 == 0); - }
REPORT_TEST_PASS(); } @@ -1474,6 +1720,7 @@ static void test_seal_mremap_move_dontunmap(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1488,6 +1735,10 @@ static void test_seal_mremap_move_dontunmap(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { /* kernel will allocate a new address */ FAIL_TEST_IF_FALSE(ret2 != MAP_FAILED); @@ -1503,6 +1754,7 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal) unsigned long size = 4 * page_size; int ret; void *ret2; + int prot;
setup_single_address(size, &ptr); FAIL_TEST_IF_FALSE(ptr != (void *)-1); @@ -1529,6 +1781,10 @@ static void test_seal_mremap_move_dontunmap_anyaddr(bool seal) if (seal) { FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); } else { /* remap success and return ptr2 */ FAIL_TEST_IF_FALSE(ret2 == ptr2); @@ -1690,9 +1946,10 @@ static void test_seal_discard_ro_anon_on_pkey(bool seal) /* sealing will take effect if PKRU deny write. */ set_pkey(pkey, PKEY_DISABLE_WRITE); ret = sys_madvise(ptr, size, MADV_DONTNEED); - if (seal) + if (seal) { FAIL_TEST_IF_FALSE(ret < 0); - else + FAIL_TEST_IF_FALSE(errno == EPERM); + } else FAIL_TEST_IF_FALSE(!ret);
/* base seal still apply. */ @@ -1876,6 +2133,15 @@ int main(int argc, char **argv) if (!pkey_supported()) ksft_print_msg("PKEY not supported\n");
+ /* + * Possible reasons: + * - unable to read /proc/pid/maps (unlikely) + * - parsing error when reading /proc/pid/maps,e.g. len is not expected. + * Is this "TOPDOWN" mapping or format change in /proc/pid/maps ? + */ + if (!get_vma_size_supported()) + ksft_exit_skip("get_vma_size not supported\n"); + ksft_set_plan(88);
test_seal_addseal();
From: Jeff Xu jeffxu@chromium.org
Add a testcase to cover all sealed madvise type.
Signed-off-by: Jeff Xu jeffxu@chromium.org --- tools/testing/selftests/mm/mseal_test.c | 30 ++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index 7198f2314f9b..6d77dc9b5442 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2121,6 +2121,32 @@ static void test_seal_madvise_nodiscard(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_discard_madvise_advice(void) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 4 * page_size; + int ret; + int sealed_advice[] = {MADV_FREE, MADV_DONTNEED, + MADV_DONTNEED_LOCKED, MADV_REMOVE, + MADV_DONTFORK, MADV_WIPEONFORK}; + int size_sealed_advice = sizeof(sealed_advice) / sizeof(int); + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + ret = seal_single_address(ptr, size); + FAIL_TEST_IF_FALSE(!ret); + + for (int i = 0; i < size_sealed_advice; i++) { + ret = sys_madvise(ptr, size, sealed_advice[i]); + FAIL_TEST_IF_FALSE(ret < 0); + FAIL_TEST_IF_FALSE(errno == EPERM); + } + + REPORT_TEST_PASS(); +} + int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2142,7 +2168,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(88); + ksft_set_plan(89);
test_seal_addseal(); test_seal_unmapped_start(); @@ -2251,5 +2277,7 @@ int main(int argc, char **argv) test_seal_discard_ro_anon_on_pkey(false); test_seal_discard_ro_anon_on_pkey(true);
+ test_seal_discard_madvise_advice(); + ksft_finished(); }
From: Jeff Xu jeffxu@chromium.org
Add a test to munmap across multiple vma ranges.
Signed-off-by: Jeff Xu jeffxu@chromium.org --- tools/testing/selftests/mm/mseal_test.c | 80 ++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index 6d77dc9b5442..e855c8ccefc3 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2147,6 +2147,81 @@ static void test_seal_discard_madvise_advice(void) REPORT_TEST_PASS(); }
+static void test_munmap_free_multiple_ranges(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 8 * page_size; + int ret; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + /* unmap one page from beginning. */ + ret = sys_munmap(ptr, page_size); + FAIL_TEST_IF_FALSE(!ret); + + /* unmap one page from middle. */ + ret = sys_munmap(ptr + 4 * page_size, page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr + page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 5 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + + /* seal the last page */ + if (seal) { + ret = sys_mseal(ptr + 7 * page_size, page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr + 1 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 5 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 7 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + } + + /* munmap all 8 pages from beginning */ + ret = sys_munmap(ptr, 8 * page_size); + if (seal) { + FAIL_TEST_IF_FALSE(ret); + + /* verify mapping are not changed */ + size = get_vma_size(ptr + 1 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 3 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 5 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 7 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 1 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + } else { + FAIL_TEST_IF_FALSE(!ret); + + for (int i = 0; i < 8; i++) { + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 0); + } + } + + REPORT_TEST_PASS(); +} + int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2168,7 +2243,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(89); + ksft_set_plan(91);
test_seal_addseal(); test_seal_unmapped_start(); @@ -2279,5 +2354,8 @@ int main(int argc, char **argv)
test_seal_discard_madvise_advice();
+ test_munmap_free_multiple_ranges(false); + test_munmap_free_multiple_ranges(true); + ksft_finished(); }
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
Signed-off-by: Jeff Xu jeffxu@chromium.org --- tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + /* ummap last 4 pages. */ + ret = sys_munmap(ptr + 8 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 8 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to expand and overwrite (MAP_FIXED) */ + ret2 = mmap(ptr, 12 * page_size, PROT_READ, + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); + if (seal) { + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(ret2 == ptr); + + REPORT_TEST_PASS(); +} + +static void test_seal_mmap_shrink_seal_middle(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to shrink and overwrite (MAP_FIXED) */ + ret2 = mmap(ptr, 7 * page_size, PROT_READ, + MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0); + if (seal) { + FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + } else + FAIL_TEST_IF_FALSE(ret2 == ptr); + + REPORT_TEST_PASS(); +} + +static void test_seal_mmap_reuse_addr(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + if (seal) { + ret = sys_mseal(ptr, size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* use mmap to change protection. */ + ret2 = mmap(ptr, size, PROT_NONE, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + + /* MAP_FIXED is not used, expect new addr */ + FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED)); + FAIL_TEST_IF_FALSE(ret2 != ptr); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == page_size); + FAIL_TEST_IF_FALSE(prot == 0x4); + + REPORT_TEST_PASS(); +} + int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91); + ksft_set_plan(97);
test_seal_addseal(); test_seal_unmapped_start(); @@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
+ test_seal_mmap_expand_seal_middle(false); + test_seal_mmap_expand_seal_middle(true); + test_seal_mmap_shrink_seal_middle(false); + test_seal_mmap_shrink_seal_middle(true); + test_seal_mmap_reuse_addr(false); + test_seal_mmap_reuse_addr(true); + ksft_finished(); }
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
This commit message is woefully small. I told you on v1 to improve the commit messages. Linus has told you to do this before.
Please actually respond to feedback. Thanks.
Signed-off-by: Jeff Xu jeffxu@chromium.org
tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal)
This test doesn't expand, doesn't do anything in the middle. It does mmap() though and relates to mseal, so that's something... this is compeltely misnamed and needs to be rethought.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
Please replace every single instance of this with an mmap(). There's literally no reason to abstract it. And munmap() what you map.
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
Pretty sure Pedro pointed out you should be checking against MAP_FAILED here. I really don't understand why the rest of your test is full of mmap()'s but for some reason you choose to abstract this one call? What?
- /* ummap last 4 pages. */
- ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
sys_munmap()? What's wrong with munmap()?
- FAIL_TEST_IF_FALSE(!ret);
Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == 8 * page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to expand and overwrite (MAP_FIXED) */
You don't really need to say MAP_FIXED, it's below.
- ret2 = mmap(ptr, 12 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
Why read-only?
You're not expanding you're overwriting. You're not doing anything in the middle.
I'm again confused about what you think you're testing here. I don't think we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten VMA?
You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() if that's what you want.
- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);
Don't do dangling else's after a big block.
- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_shrink_seal_middle(bool seal)
What's going on in the 'middle'? This test doesn't shrink, it overwrites the beginning of a sealed VMA?
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to shrink and overwrite (MAP_FIXED) */
What exactly are you shrinking? You're overwriting the start of the vma?
What is this testing that is different from the previous test? This seems useless honestly.
- ret2 = mmap(ptr, 7 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);
What the hell is this comparison to magic numbers? This is ridiculous. What's wrong with PROT_xxx??
size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);
Err dude, you're doing this twice?
So what are we testing here exactly? That we got a VMA split? This is err... why are we asserting this?
- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_reuse_addr(bool seal)
This is wrong, you're not reusing anything. This test is useless.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr, size);FAIL_TEST_IF_FALSE(!ret);
We could avoid this horrid ret, ret2 naming if you just did:
FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
- }
- /* use mmap to change protection. */
- ret2 = mmap(ptr, size, PROT_NONE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
How are you using mmap to change the protection when you're providing a hint to the address to use? You're not changing any protection at all!
You're allocating an entirely new VMA hinting that you want it near ptr. Please read the man page for mmap():
If addr is NULL, then the kernel chooses the (page-aligned) address at which to create the mapping; this is the most portable method of creating a new mapping. If addr is not NULL, then the kernel takes it as a hint about where to place the mapping; on Linux, the kernel will pick a nearby page boundary (but always above or equal to the value specified by /proc/sys/vm/mmap_min_addr) and attempt to create the mapping there. If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint. The address of the new mapping is returned as the result of the call.
- /* MAP_FIXED is not used, expect new addr */
- FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
This is beyond horrible. You really have to add more asserts.
Also you're expecting a new address here, so again, what on earth are you asserting? That we can mmap()?
- FAIL_TEST_IF_FALSE(ret2 != ptr);
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- REPORT_TEST_PASS();
+}
int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91);
- ksft_set_plan(97);
I'm guessing this is the number of tests, but I mean this is horrible. Is there not a better way of doing this?
test_seal_addseal(); test_seal_unmapped_start(); @@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
- test_seal_mmap_expand_seal_middle(false);
- test_seal_mmap_expand_seal_middle(true);
- test_seal_mmap_shrink_seal_middle(false);
- test_seal_mmap_shrink_seal_middle(true);
- test_seal_mmap_reuse_addr(false);
- test_seal_mmap_reuse_addr(true);
- ksft_finished();
}
2.46.0.469.g59c65b2a67-goog
On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
This commit message is woefully small. I told you on v1 to improve the commit messages. Linus has told you to do this before.
Please actually respond to feedback. Thanks.
Signed-off-by: Jeff Xu jeffxu@chromium.org
tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal)
This test doesn't expand, doesn't do anything in the middle. It does mmap() though and relates to mseal, so that's something... this is compeltely misnamed and needs to be rethought.
OK correction - it _seals_ in the middle. The remained of the criticism remains, and this is rather confusing... and I continue to wonder what the purpose of this is?
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
Please replace every single instance of this with an mmap(). There's literally no reason to abstract it. And munmap() what you map.
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
Pretty sure Pedro pointed out you should be checking against MAP_FAILED here. I really don't understand why the rest of your test is full of mmap()'s but for some reason you choose to abstract this one call? What?
- /* ummap last 4 pages. */
- ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
sys_munmap()? What's wrong with munmap()?
- FAIL_TEST_IF_FALSE(!ret);
Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == 8 * page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to expand and overwrite (MAP_FIXED) */
You don't really need to say MAP_FIXED, it's below.
- ret2 = mmap(ptr, 12 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);Why read-only?
You're not expanding you're overwriting. You're not doing anything in the middle.
I'm again confused about what you think you're testing here. I don't think we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten VMA?
You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() if that's what you want.
- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);Don't do dangling else's after a big block.
- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_shrink_seal_middle(bool seal)
What's going on in the 'middle'? This test doesn't shrink, it overwrites the beginning of a sealed VMA?
Correction - the middle is sealed. Other points remain.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to shrink and overwrite (MAP_FIXED) */
What exactly are you shrinking? You're overwriting the start of the vma?
What is this testing that is different from the previous test? This seems useless honestly.
- ret2 = mmap(ptr, 7 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);What the hell is this comparison to magic numbers? This is ridiculous. What's wrong with PROT_xxx??
size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);Err dude, you're doing this twice?
So what are we testing here exactly? That we got a VMA split? This is err... why are we asserting this?
I guess, that we can't overwrite a sealed bit of a VMA at the end. But again this feels entirely redundant. For this kind of thing to fail would mean the whole VMA machinery is broken.
- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_reuse_addr(bool seal)
This is wrong, you're not reusing anything. This test is useless.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr, size);FAIL_TEST_IF_FALSE(!ret);We could avoid this horrid ret, ret2 naming if you just did:
FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
- }
- /* use mmap to change protection. */
- ret2 = mmap(ptr, size, PROT_NONE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);How are you using mmap to change the protection when you're providing a hint to the address to use? You're not changing any protection at all!
You're allocating an entirely new VMA hinting that you want it near ptr. Please read the man page for mmap():
If addr is NULL, then the kernel chooses the (page-aligned) address at which to create the mapping; this is the most portable method of creating a new mapping. If addr is not NULL, then the kernel takes it as a hint about where to place the mapping; on Linux, the kernel will pick a nearby page boundary (but always above or equal to the value specified by /proc/sys/vm/mmap_min_addr) and attempt to create the mapping there. If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint. The address of the new mapping is returned as the result of the call.
- /* MAP_FIXED is not used, expect new addr */
- FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
This is beyond horrible. You really have to add more asserts.
Also you're expecting a new address here, so again, what on earth are you asserting? That we can mmap()?
- FAIL_TEST_IF_FALSE(ret2 != ptr);
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- REPORT_TEST_PASS();
+}
int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91);
- ksft_set_plan(97);
I'm guessing this is the number of tests, but I mean this is horrible. Is there not a better way of doing this?
test_seal_addseal(); test_seal_unmapped_start(); @@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
- test_seal_mmap_expand_seal_middle(false);
- test_seal_mmap_expand_seal_middle(true);
- test_seal_mmap_shrink_seal_middle(false);
- test_seal_mmap_shrink_seal_middle(true);
- test_seal_mmap_reuse_addr(false);
- test_seal_mmap_reuse_addr(true);
- ksft_finished();
}
2.46.0.469.g59c65b2a67-goog
On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
This commit message is woefully small. I told you on v1 to improve the commit messages. Linus has told you to do this before.
Please actually respond to feedback. Thanks.
Signed-off-by: Jeff Xu jeffxu@chromium.org
tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal)
This test doesn't expand, doesn't do anything in the middle. It does mmap() though and relates to mseal, so that's something... this is compeltely misnamed and needs to be rethought.
OK correction - it _seals_ in the middle. The remained of the criticism remains, and this is rather confusing... and I continue to wonder what the purpose of this is?
It expands the size (start from ptr).
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
Please replace every single instance of this with an mmap(). There's literally no reason to abstract it. And munmap() what you map.
No, we need to abstract it. In addition to the mmap, it also allocates an additional two blocks before and after the allocated memory, to avoid auto-merging, so we can use get_vma_size.
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
Pretty sure Pedro pointed out you should be checking against MAP_FAILED here. I really don't understand why the rest of your test is full of mmap()'s but for some reason you choose to abstract this one call? What?
- /* ummap last 4 pages. */
- ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
sys_munmap()? What's wrong with munmap()?
- FAIL_TEST_IF_FALSE(!ret);
Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().
ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks related to self-test infra, such as count how many tests are failing.
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == 8 * page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to expand and overwrite (MAP_FIXED) */
You don't really need to say MAP_FIXED, it's below.
Adding a comment here to help reviewers.
- ret2 = mmap(ptr, 12 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);Why read-only?
You're not expanding you're overwriting. You're not doing anything in the middle.
The MAP_FIXED is overwriting. It also expands the address range (start from ptr) from 8 to 12 pages.
I'm again confused about what you think you're testing here. I don't think we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten VMA?
You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() if that's what you want.
- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);Don't do dangling else's after a big block.
patch passed the checkpatch.pl for style check.
- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_shrink_seal_middle(bool seal)
What's going on in the 'middle'? This test doesn't shrink, it overwrites the beginning of a sealed VMA?
Correction - the middle is sealed. Other points remain.
The mmap attempts to shrink the address range from 12 pages to 8 pages.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to shrink and overwrite (MAP_FIXED) */
What exactly are you shrinking? You're overwriting the start of the vma?
What is this testing that is different from the previous test? This seems useless honestly.
Again, as above, one test is expanding, the other test is shrinking. Please take a look at mmap parameters and steps before mmap call.
- ret2 = mmap(ptr, 7 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);What the hell is this comparison to magic numbers? This is ridiculous. What's wrong with PROT_xxx??
The PROT_xxx can't be used here. get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);Err dude, you're doing this twice?
The second get_vma_size should be (ptr + 8 * page_size) I will update that.
So what are we testing here exactly? That we got a VMA split? This is err... why are we asserting this?
I guess, that we can't overwrite a sealed bit of a VMA at the end. But again this feels entirely redundant. For this kind of thing to fail would mean the whole VMA machinery is broken.
The test is testing mmap(MAP_FIXED), since it can be used to overwrite the sealed memory range (without sealing), then there is a variant of expand/shrink.
- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_reuse_addr(bool seal)
This is wrong, you're not reusing anything. This test is useless.
The ptr is reused as a hint.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr, size);FAIL_TEST_IF_FALSE(!ret);We could avoid this horrid ret, ret2 naming if you just did:
FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
- }
- /* use mmap to change protection. */
- ret2 = mmap(ptr, size, PROT_NONE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);How are you using mmap to change the protection when you're providing a hint to the address to use? You're not changing any protection at all!
It is necessary to add the this tests to make sure mseal is behave as it should be, which is !MAP_FIXED case, new address will be allocated, instead of fail of mmap()
You're allocating an entirely new VMA hinting that you want it near ptr. Please read the man page for mmap():
If addr is NULL, then the kernel chooses the (page-aligned) address at which to create the mapping; this is the most portable method of creating a new mapping. If addr is not NULL, then the kernel takes it as a hint about where to place the mapping; on Linux, the kernel will pick a nearby page boundary (but always above or equal to the value specified by /proc/sys/vm/mmap_min_addr) and attempt to create the mapping there. If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint. The address of the new mapping is returned as the result of the call.
- /* MAP_FIXED is not used, expect new addr */
- FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
This is beyond horrible. You really have to add more asserts.
Again assert is not recommended by self_test
Also you're expecting a new address here, so again, what on earth are you asserting? That we can mmap()?
- FAIL_TEST_IF_FALSE(ret2 != ptr);
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- REPORT_TEST_PASS();
+}
int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91);
- ksft_set_plan(97);
I'm guessing this is the number of tests, but I mean this is horrible. Is there not a better way of doing this?
Again, this is recommended by self-test.
test_seal_addseal(); test_seal_unmapped_start();@@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
- test_seal_mmap_expand_seal_middle(false);
- test_seal_mmap_expand_seal_middle(true);
- test_seal_mmap_shrink_seal_middle(false);
- test_seal_mmap_shrink_seal_middle(true);
- test_seal_mmap_reuse_addr(false);
- test_seal_mmap_reuse_addr(true);
- ksft_finished();
}
2.46.0.469.g59c65b2a67-goog
On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
Hi Jeff, I really want to find a constructive way forward, but you've basically ignored more or less everything I've said here.
I could respond again to each of your points here, but - from my point of view - if your response to 'what is this even testing?' is to just repeat in effect the name of the test - we will be stuck in a loop, which will be exited with a NACK. I don't want this.
The majority of these tests, from a VMA/mmap point of view, appear to me to be essentially testing 'does basic mmap functionality work correctly', which isn't testing mseal.
Look - I appreciate your commitment to testing (see my work on mm/vma.c - I care passionately about testing) - but we must make sure we are actually testing what we mean to.
So I suggest as a constructive way forward - firstly, submit a regression test for the change Liam wrapped into his series regarding the -EPERM thing.
This should go in uncontroversially, I will take the time to review it and I don't see why that shouldn't be relatively straight forward. I will drop the concerns about things like test macros etc. for that.
Then after that, I suggest we have a discussion about - at a higher level - what it is you want to test. And then between me, you, Liam and Pedro - ahead of time, list out the tests that we want, with all of us reaching consensus.
I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point too - I may be missing something, but I cannot for the life me understand why we have to assert negations only, and other self tests do not do this.
I have replied to a few sample points below.
All of us simply want to help make sure mseal works as well as it can, this is the only motivation at play here.
Hope you have a great weekend!
Cheers, Lorenzo
This commit message is woefully small. I told you on v1 to improve the commit messages. Linus has told you to do this before.
Please actually respond to feedback. Thanks.
Signed-off-by: Jeff Xu jeffxu@chromium.org
tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal)
This test doesn't expand, doesn't do anything in the middle. It does mmap() though and relates to mseal, so that's something... this is compeltely misnamed and needs to be rethought.
OK correction - it _seals_ in the middle. The remained of the criticism remains, and this is rather confusing... and I continue to wonder what the purpose of this is?
It expands the size (start from ptr).
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
Please replace every single instance of this with an mmap(). There's literally no reason to abstract it. And munmap() what you map.
No, we need to abstract it. In addition to the mmap, it also allocates an additional two blocks before and after the allocated memory, to avoid auto-merging, so we can use get_vma_size.
It doesn't?
static void setup_single_address(int size, void **ptrOut) { void *ptr;
ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); *ptrOut = ptr; }
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
Pretty sure Pedro pointed out you should be checking against MAP_FAILED here. I really don't understand why the rest of your test is full of mmap()'s but for some reason you choose to abstract this one call? What?
- /* ummap last 4 pages. */
- ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
sys_munmap()? What's wrong with munmap()?
- FAIL_TEST_IF_FALSE(!ret);
Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().
ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks related to self-test infra, such as count how many tests are failing.
Can you please point me to where it says you should implement your own macro that only tests the negation of an expression?
I have found other self tests that do.
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == 8 * page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to expand and overwrite (MAP_FIXED) */
You don't really need to say MAP_FIXED, it's below.
Adding a comment here to help reviewers.
- ret2 = mmap(ptr, 12 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);Why read-only?
You're not expanding you're overwriting. You're not doing anything in the middle.
The MAP_FIXED is overwriting. It also expands the address range (start from ptr) from 8 to 12 pages.
I'm again confused about what you think you're testing here. I don't think we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten VMA?
You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() if that's what you want.
- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);Don't do dangling else's after a big block.
patch passed the checkpatch.pl for style check.
- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_shrink_seal_middle(bool seal)
What's going on in the 'middle'? This test doesn't shrink, it overwrites the beginning of a sealed VMA?
Correction - the middle is sealed. Other points remain.
The mmap attempts to shrink the address range from 12 pages to 8 pages.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to shrink and overwrite (MAP_FIXED) */
What exactly are you shrinking? You're overwriting the start of the vma?
What is this testing that is different from the previous test? This seems useless honestly.
Again, as above, one test is expanding, the other test is shrinking. Please take a look at mmap parameters and steps before mmap call.
- ret2 = mmap(ptr, 7 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);What the hell is this comparison to magic numbers? This is ridiculous. What's wrong with PROT_xxx??
The PROT_xxx can't be used here. get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);Err dude, you're doing this twice?
The second get_vma_size should be (ptr + 8 * page_size) I will update that.
So what are we testing here exactly? That we got a VMA split? This is err... why are we asserting this?
I guess, that we can't overwrite a sealed bit of a VMA at the end. But again this feels entirely redundant. For this kind of thing to fail would mean the whole VMA machinery is broken.
The test is testing mmap(MAP_FIXED), since it can be used to overwrite the sealed memory range (without sealing), then there is a variant of expand/shrink.
- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_reuse_addr(bool seal)
This is wrong, you're not reusing anything. This test is useless.
The ptr is reused as a hint.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr, size);FAIL_TEST_IF_FALSE(!ret);We could avoid this horrid ret, ret2 naming if you just did:
FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
- }
- /* use mmap to change protection. */
- ret2 = mmap(ptr, size, PROT_NONE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);How are you using mmap to change the protection when you're providing a hint to the address to use? You're not changing any protection at all!
It is necessary to add the this tests to make sure mseal is behave as it should be, which is !MAP_FIXED case, new address will be allocated, instead of fail of mmap()
You're allocating an entirely new VMA hinting that you want it near ptr. Please read the man page for mmap():
If addr is NULL, then the kernel chooses the (page-aligned) address at which to create the mapping; this is the most portable method of creating a new mapping. If addr is not NULL, then the kernel takes it as a hint about where to place the mapping; on Linux, the kernel will pick a nearby page boundary (but always above or equal to the value specified by /proc/sys/vm/mmap_min_addr) and attempt to create the mapping there. If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint. The address of the new mapping is returned as the result of the call.
- /* MAP_FIXED is not used, expect new addr */
- FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
This is beyond horrible. You really have to add more asserts.
Again assert is not recommended by self_test
Also you're expecting a new address here, so again, what on earth are you asserting? That we can mmap()?
- FAIL_TEST_IF_FALSE(ret2 != ptr);
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- REPORT_TEST_PASS();
+}
int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91);
- ksft_set_plan(97);
I'm guessing this is the number of tests, but I mean this is horrible. Is there not a better way of doing this?
Again, this is recommended by self-test.
test_seal_addseal(); test_seal_unmapped_start();@@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
- test_seal_mmap_expand_seal_middle(false);
- test_seal_mmap_expand_seal_middle(true);
- test_seal_mmap_shrink_seal_middle(false);
- test_seal_mmap_shrink_seal_middle(true);
- test_seal_mmap_reuse_addr(false);
- test_seal_mmap_reuse_addr(true);
- ksft_finished();
}
2.46.0.469.g59c65b2a67-goog
On Sat, Sep 07, 2024 at 08:27:52PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
Hi Jeff, I really want to find a constructive way forward, but you've basically ignored more or less everything I've said here.
I could respond again to each of your points here, but - from my point of view - if your response to 'what is this even testing?' is to just repeat in effect the name of the test - we will be stuck in a loop, which will be exited with a NACK. I don't want this.
The majority of these tests, from a VMA/mmap point of view, appear to me to be essentially testing 'does basic mmap functionality work correctly', which isn't testing mseal.
Look - I appreciate your commitment to testing (see my work on mm/vma.c - I care passionately about testing) - but we must make sure we are actually testing what we mean to.
So I suggest as a constructive way forward - firstly, submit a regression test for the change Liam wrapped into his series regarding the -EPERM thing.
This should go in uncontroversially, I will take the time to review it and I don't see why that shouldn't be relatively straight forward. I will drop the concerns about things like test macros etc. for that.
Then after that, I suggest we have a discussion about - at a higher level - what it is you want to test. And then between me, you, Liam and Pedro - ahead of time, list out the tests that we want, with all of us reaching consensus.
Hi,
I agree with most of the points. Sitting down here to write unofficial guidelines for mseal behavior.
mseal should seal regions and mark them immutable, which means their protection and contents (ish?) (not _only_ backing mapping, but also contents in general (see madvise below)) should not be changed throughout the lifetime of the address space.
For the general syscall interface, this means: 1) mprotect and munmap need to be blocked on mseal regions. 1a) munmap _cannot_ tolerate partial failure, per POSIX. 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
2) Most madvise calls are allowed, except for destructive operations on read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care about blocking these ops if we can do it manually with e.g memset) 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly different. We probably do want to block these on MAP_PRIVATE file mappings, as to block stuff like madvise MADV_DONTNEED on program rodata. 2b) We take into account pkeys when doing the permission checks.
3) mremap is not allowed as we'd change the "contents" of the old region. 3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. We already informally allow expansion if e.g mmapping after it + mseal.
4) mlock and msync are allowed.
5) mseal is blocked.
6) Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed. 6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
7) FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) should be disallowed (?) 7a) This currently does not happen, and seems like a large hole? But disallowing this would probably severely break ptrace if the ELF sealing plans come to fruition.
When we say "disallowed", we usually (apart from munmap) allow for partial failure. This means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves into a corner - this is strictly "undefined behavior". The msealed regions themselves will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly does not make sense to block operations unless they affect a VMA entirely, and that would also force VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
Do I have everything right? Am I missing anything?
On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato pedro.falcato@gmail.com wrote:
Hi,
I agree with most of the points. Sitting down here to write unofficial guidelines for mseal behavior.
mseal should seal regions and mark them immutable, which means their protection and contents (ish?) (not _only_ backing mapping, but also contents in general (see madvise below)) should not be changed throughout the lifetime of the address space.
For the general syscall interface, this means:
- mprotect and munmap need to be blocked on mseal regions.
1a) munmap _cannot_ tolerate partial failure, per POSIX. 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
- Most madvise calls are allowed, except for destructive operations on
read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care about blocking these ops if we can do it manually with e.g memset) 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly different. We probably do want to block these on MAP_PRIVATE file mappings, as to block stuff like madvise MADV_DONTNEED on program rodata. 2b) We take into account pkeys when doing the permission checks.
- mremap is not allowed as we'd change the "contents" of the old region.
3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. We already informally allow expansion if e.g mmapping after it + mseal.
mlock and msync are allowed.
mseal is blocked.
Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
- FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) should be disallowed (?)
7a) This currently does not happen, and seems like a large hole? But disallowing this would probably severely break ptrace if the ELF sealing plans come to fruition.
When we say "disallowed", we usually (apart from munmap) allow for partial failure. This means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves into a corner - this is strictly "undefined behavior". The msealed regions themselves will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly does not make sense to block operations unless they affect a VMA entirely, and that would also force VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
Do I have everything right? Am I missing anything?
Small addendum: file write, truncate and hole punching might also matter for sealed file-backed regions, as these change the region's contents, but we probably want to rely on file write permissions to protect against this (as we already do). Any other solution is probably terrible and probably endlessly NAK'd by fs folks, but it does mean sealed regions aren't really immutable if you or the attacker can write to the file.
Hi Pedro
On Sun, Sep 8, 2024 at 2:56 PM Pedro Falcato pedro.falcato@gmail.com wrote:
On Sun, Sep 8, 2024 at 10:35 PM Pedro Falcato pedro.falcato@gmail.com wrote:
Hi,
I agree with most of the points. Sitting down here to write unofficial guidelines for mseal behavior.
mseal should seal regions and mark them immutable, which means their protection and contents (ish?) (not _only_ backing mapping, but also contents in general (see madvise below)) should not be changed throughout the lifetime of the address space.
For the general syscall interface, this means:
- mprotect and munmap need to be blocked on mseal regions.
1a) munmap _cannot_ tolerate partial failure, per POSIX. 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
- Most madvise calls are allowed, except for destructive operations on
read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care about blocking these ops if we can do it manually with e.g memset) 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly different. We probably do want to block these on MAP_PRIVATE file mappings, as to block stuff like madvise MADV_DONTNEED on program rodata. 2b) We take into account pkeys when doing the permission checks.
- mremap is not allowed as we'd change the "contents" of the old region.
3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. We already informally allow expansion if e.g mmapping after it + mseal.
mlock and msync are allowed.
mseal is blocked.
Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
- FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) should be disallowed (?)
7a) This currently does not happen, and seems like a large hole? But disallowing this would probably severely break ptrace if the ELF sealing plans come to fruition.
When we say "disallowed", we usually (apart from munmap) allow for partial failure. This means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves into a corner - this is strictly "undefined behavior". The msealed regions themselves will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly does not make sense to block operations unless they affect a VMA entirely, and that would also force VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
Do I have everything right? Am I missing anything?
Small addendum: file write, truncate and hole punching might also matter for sealed file-backed regions, as these change the region's contents, but we probably want to rely on file write permissions to protect against this (as we already do). Any other solution is probably terrible and probably endlessly NAK'd by fs folks, but it does mean sealed regions aren't really immutable if you or the attacker can write to the file.
Right. The mseal protects the control-data of VMA (e.g. prot), it doesn't do anything more than that. The file permission relies on dac/mac control.
-Jeff
-- Pedro
Hi Pedro
On Sun, Sep 8, 2024 at 2:35 PM Pedro Falcato pedro.falcato@gmail.com wrote:
I agree with most of the points. Sitting down here to write unofficial guidelines for mseal behavior.
mseal should seal regions and mark them immutable, which means their protection and contents (ish?) (not _only_ backing mapping, but also contents in general (see madvise below)) should not be changed throughout the lifetime of the address space.
For the general syscall interface, this means:
- mprotect and munmap need to be blocked on mseal regions.
1a) munmap _cannot_ tolerate partial failure, per POSIX. 2b) mmap MAP_FIXED counts as an unmap operation and also needs to be blocked and return -EPERM.
- Most madvise calls are allowed, except for destructive operations on
read-only anonymous _pages_ (MADV_DONTNEED is destructive for anon, but we also don't care about blocking these ops if we can do it manually with e.g memset) 2a) The current implementation only blocks discard on anonymous _regions_, which is slightly different. We probably do want to block these on MAP_PRIVATE file mappings, as to block stuff like madvise MADV_DONTNEED on program rodata. 2b) We take into account pkeys when doing the permission checks.
- mremap is not allowed as we'd change the "contents" of the old region.
3a) Should mremap expansion be allowed? aka only block moving and shrinking, but allow expansion. We already informally allow expansion if e.g mmapping after it + mseal.
mlock and msync are allowed.
mseal is blocked.
mseal is not blocked, i.e. seal on an already sealed memory is no-op. This is described in mseal.rst [1]
[1] https://github.com/torvalds/linux/blob/master/Documentation/userspace-api/ms...
- Other miscellaneous syscalls (mbind, etc) that do not change contents in any way, are allowed.
6a) This obviously means PTEs can change as long as the contents don't. Swapping is also ok.
- FOLL_FORCE (kernel-internal speak, more commonly seen as ptrace and /proc/self/mem from userspace) should be disallowed (?)
7a) This currently does not happen, and seems like a large hole? But disallowing this would probably severely break ptrace if the ELF sealing plans come to fruition.
Jann Horn pointed out FOLL_FORCE during RFC [2], and this is in mseal.rst too.
In short, FOLL_FORCE is not covered by mseal. On ChromeOS, FOLL_FORCE is disabled. Recently, Adrian Ratiu upstreamed that [3]
[2] https://lore.kernel.org/lkml/CAG48ez3ShUYey+ZAFsU2i1RpQn0a5eOs2hzQ426FkcgnfU... [3] https://lore.kernel.org/lkml/20240802080225.89408-1-adrian.ratiu@collabora.c...
-Jeff
When we say "disallowed", we usually (apart from munmap) allow for partial failure. This means getting an -EPERM while part of the call succeeded, if we e.g mprotect a region consisting of [NORMAL VMA][SEALED VMA]. We do not want to test for this, because we do not want to paint ourselves into a corner - this is strictly "undefined behavior". The msealed regions themselves will never be touched in such cases. (we do however want to test munmap operation atomicity, but this is also kind of a munmap-related test, and might not really be something we really want in the mseal tests)
Kernel-internal wise: The VMA and PTE modifications resulting from the above operations are blocked. Sealed VMAs allow splitting and merging; there was contention about the splitting issue, but it truly does not make sense to block operations unless they affect a VMA entirely, and that would also force VMA merging to be ABI ("vma_merge isn't merging these two regions and now my madvise works/doesn't work :(").
Do I have everything right? Am I missing anything?
-- Pedro
Hi Lorenzo
On Sat, Sep 7, 2024 at 12:28 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Fri, Aug 30, 2024 at 04:57:26PM GMT, Jeff Xu wrote:
On Fri, Aug 30, 2024 at 12:23 PM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
On Fri, Aug 30, 2024 at 07:43:12PM GMT, Lorenzo Stoakes wrote:
On Fri, Aug 30, 2024 at 06:02:36PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mmap for Expand/shrink across sealed vmas (MAP_FIXED) Reuse the same address in !MAP_FIXED case.
Hi Jeff, I really want to find a constructive way forward, but you've basically ignored more or less everything I've said here.
I could respond again to each of your points here, but - from my point of view - if your response to 'what is this even testing?' is to just repeat in effect the name of the test - we will be stuck in a loop, which will be exited with a NACK. I don't want this.
The majority of these tests, from a VMA/mmap point of view, appear to me to be essentially testing 'does basic mmap functionality work correctly', which isn't testing mseal.
Look - I appreciate your commitment to testing (see my work on mm/vma.c - I care passionately about testing) - but we must make sure we are actually testing what we mean to.
I'm also passionate about testing :-)
Maybe there is a mis-understanding ? I explained the purpose of this patch set in various responses, maybe not directly to your email though.
Even though the number of lines is large in these patches, its main intention is to test Pedro's in-place change (from can_modify_mm to can_modify_vma). Before this patch, the test had a common pattern: setup memory layout, seal the memory, perform a few mm-api steps, verify return code (not zero). Because of the nature of out-of-loop, it is sufficient to just verify the error code in a few cases.
With Pedro's in-loop change, the sealing check happens later in the stack, thus there are more things and scenarios to verify. And there were feedback to me during in-loop change that selftest should be extensive enough to discover all regressions. Even though this viewpoint is subject to debate. Since none would want to do it, I thought I would just do it.
So the Patch V3 1/5 is dedicated entirely to increasing the verification for existing scenarios, this including checking return code code, vma-size, etc after mm api return.
Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop change, we discovered a bug in unmap(), and unmap() is not atomic. This leads to 4/5(mmap), 5/5(mremap), which calls munmap(). In addition, I add scenarios to cover cross-multiple-vma cases.
The high-level goal of mseal test are two folds: 1> make sure sealing is working correctly under different scenarios, i.e. sealed mapping are not modified. 2> For unsealed memory, added mseal code doesn't regress on regular mm API.
The goal 2 is as important as 1, that is why tests usually are done in two phases, one with sealing, the other without.
So I suggest as a constructive way forward - firstly, submit a regression test for the change Liam wrapped into his series regarding the -EPERM thing.
I could work on this (to split the patch further) if this helps acceptance of the patch series.
However, since the merge window is closer, everyone is busy, and it is not really urgent to get it merged. the added tests already passed in the linux-next branch, we could wait till after merge-window to review/perfect those tests.
This should go in uncontroversially, I will take the time to review it and I don't see why that shouldn't be relatively straight forward. I will drop the concerns about things like test macros etc. for that.
Then after that, I suggest we have a discussion about - at a higher level - what it is you want to test. And then between me, you, Liam and Pedro - ahead of time, list out the tests that we want, with all of us reaching consensus.
I also suggest we figure out this FAIL_TEST_IF_FALSE() thing at this point too - I may be missing something, but I cannot for the life me understand why we have to assert negations only, and other self tests do not do this.
My most test-infra related comments comes from Muhammad Usama Anjum (added into this email), e.g. assert is not recommended.[1] ,
[1] https://lore.kernel.org/all/148fc789-3c03-4490-a653-6a4e58f336b6@collabora.c...
I have replied to a few sample points below.
All of us simply want to help make sure mseal works as well as it can, this is the only motivation at play here.
Hope you have a great weekend!
Thanks Hope a great weekend too ! -Jeff
-Jeff
Cheers, Lorenzo
This commit message is woefully small. I told you on v1 to improve the commit messages. Linus has told you to do this before.
Please actually respond to feedback. Thanks.
Signed-off-by: Jeff Xu jeffxu@chromium.org
tools/testing/selftests/mm/mseal_test.c | 126 +++++++++++++++++++++++- 1 file changed, 125 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index e855c8ccefc3..3516389034a7 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2222,6 +2222,123 @@ static void test_munmap_free_multiple_ranges(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mmap_expand_seal_middle(bool seal)
This test doesn't expand, doesn't do anything in the middle. It does mmap() though and relates to mseal, so that's something... this is compeltely misnamed and needs to be rethought.
OK correction - it _seals_ in the middle. The remained of the criticism remains, and this is rather confusing... and I continue to wonder what the purpose of this is?
It expands the size (start from ptr).
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
Please replace every single instance of this with an mmap(). There's literally no reason to abstract it. And munmap() what you map.
No, we need to abstract it. In addition to the mmap, it also allocates an additional two blocks before and after the allocated memory, to avoid auto-merging, so we can use get_vma_size.
It doesn't?
static void setup_single_address(int size, void **ptrOut) { void *ptr;
ptr = mmap(NULL, size, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); *ptrOut = ptr;}
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
Pretty sure Pedro pointed out you should be checking against MAP_FAILED here. I really don't understand why the rest of your test is full of mmap()'s but for some reason you choose to abstract this one call? What?
- /* ummap last 4 pages. */
- ret = sys_munmap(ptr + 8 * page_size, 4 * page_size);
sys_munmap()? What's wrong with munmap()?
- FAIL_TEST_IF_FALSE(!ret);
Why do we not have a FAIL_TEST_IF_TRUE()? This is crazy.
Would be nice to have something human-readable like ASSERT_EQ() or ASSERT_TRUE() or ASSERT_FALSE().
ASSERT_EQ and ASSERT_TURE are not recommended by the self-test. The FAIL_TEST_IF_FAIL wrap will take care of some of the admin tasks related to self-test infra, such as count how many tests are failing.
Can you please point me to where it says you should implement your own macro that only tests the negation of an expression?
I have found other self tests that do.
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == 8 * page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to expand and overwrite (MAP_FIXED) */
You don't really need to say MAP_FIXED, it's below.
Adding a comment here to help reviewers.
- ret2 = mmap(ptr, 12 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);Why read-only?
You're not expanding you're overwriting. You're not doing anything in the middle.
The MAP_FIXED is overwriting. It also expands the address range (start from ptr) from 8 to 12 pages.
I'm again confused about what you think you're testing here. I don't think we need an arbitrary MAP_FIXED mmap() at a size larger than the overwritten VMA?
You just need a single instance of a MAP_FIXED mmap() over a sealed mmap() if that's what you want.
- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);Don't do dangling else's after a big block.
patch passed the checkpatch.pl for style check.
- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_shrink_seal_middle(bool seal)
What's going on in the 'middle'? This test doesn't shrink, it overwrites the beginning of a sealed VMA?
Correction - the middle is sealed. Other points remain.
The mmap attempts to shrink the address range from 12 pages to 8 pages.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = 12 * page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr + 4 * page_size, 4 * page_size);FAIL_TEST_IF_FALSE(!ret);- }
- /* use mmap to shrink and overwrite (MAP_FIXED) */
What exactly are you shrinking? You're overwriting the start of the vma?
What is this testing that is different from the previous test? This seems useless honestly.
Again, as above, one test is expanding, the other test is shrinking. Please take a look at mmap parameters and steps before mmap call.
- ret2 = mmap(ptr, 7 * page_size, PROT_READ,
MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);- if (seal) {
FAIL_TEST_IF_FALSE(ret2 == MAP_FAILED);FAIL_TEST_IF_FALSE(errno == EPERM);size = get_vma_size(ptr, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);What the hell is this comparison to magic numbers? This is ridiculous. What's wrong with PROT_xxx??
The PROT_xxx can't be used here. get_vma_size doesn't return PROT_ type, i.e. the bit sequence is different.
size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);size = get_vma_size(ptr + 4 * page_size, &prot);FAIL_TEST_IF_FALSE(size == 4 * page_size);FAIL_TEST_IF_FALSE(prot == 0x4);Err dude, you're doing this twice?
The second get_vma_size should be (ptr + 8 * page_size) I will update that.
So what are we testing here exactly? That we got a VMA split? This is err... why are we asserting this?
I guess, that we can't overwrite a sealed bit of a VMA at the end. But again this feels entirely redundant. For this kind of thing to fail would mean the whole VMA machinery is broken.
The test is testing mmap(MAP_FIXED), since it can be used to overwrite the sealed memory range (without sealing), then there is a variant of expand/shrink.
- } else
FAIL_TEST_IF_FALSE(ret2 == ptr);- REPORT_TEST_PASS();
+}
+static void test_seal_mmap_reuse_addr(bool seal)
This is wrong, you're not reusing anything. This test is useless.
The ptr is reused as a hint.
+{
- void *ptr;
- unsigned long page_size = getpagesize();
- unsigned long size = page_size;
- int ret;
- void *ret2;
- int prot;
- setup_single_address(size, &ptr);
- FAIL_TEST_IF_FALSE(ptr != (void *)-1);
- if (seal) {
ret = sys_mseal(ptr, size);FAIL_TEST_IF_FALSE(!ret);We could avoid this horrid ret, ret2 naming if you just did:
FAIL_TEST_IF_FALSE(sys_mseal(ptr, size));
- }
- /* use mmap to change protection. */
- ret2 = mmap(ptr, size, PROT_NONE,
MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);How are you using mmap to change the protection when you're providing a hint to the address to use? You're not changing any protection at all!
It is necessary to add the this tests to make sure mseal is behave as it should be, which is !MAP_FIXED case, new address will be allocated, instead of fail of mmap()
You're allocating an entirely new VMA hinting that you want it near ptr. Please read the man page for mmap():
If addr is NULL, then the kernel chooses the (page-aligned) address at which to create the mapping; this is the most portable method of creating a new mapping. If addr is not NULL, then the kernel takes it as a hint about where to place the mapping; on Linux, the kernel will pick a nearby page boundary (but always above or equal to the value specified by /proc/sys/vm/mmap_min_addr) and attempt to create the mapping there. If another mapping already exists there, the kernel picks a new address that may or may not depend on the hint. The address of the new mapping is returned as the result of the call.
- /* MAP_FIXED is not used, expect new addr */
- FAIL_TEST_IF_FALSE(!(ret2 == MAP_FAILED));
This is beyond horrible. You really have to add more asserts.
Again assert is not recommended by self_test
Also you're expecting a new address here, so again, what on earth are you asserting? That we can mmap()?
- FAIL_TEST_IF_FALSE(ret2 != ptr);
- size = get_vma_size(ptr, &prot);
- FAIL_TEST_IF_FALSE(size == page_size);
- FAIL_TEST_IF_FALSE(prot == 0x4);
- REPORT_TEST_PASS();
+}
int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2243,7 +2360,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(91);
- ksft_set_plan(97);
I'm guessing this is the number of tests, but I mean this is horrible. Is there not a better way of doing this?
Again, this is recommended by self-test.
test_seal_addseal(); test_seal_unmapped_start();@@ -2357,5 +2474,12 @@ int main(int argc, char **argv) test_munmap_free_multiple_ranges(false); test_munmap_free_multiple_ranges(true);
- test_seal_mmap_expand_seal_middle(false);
- test_seal_mmap_expand_seal_middle(true);
- test_seal_mmap_shrink_seal_middle(false);
- test_seal_mmap_shrink_seal_middle(true);
- test_seal_mmap_reuse_addr(false);
- test_seal_mmap_reuse_addr(true);
- ksft_finished();
}
2.46.0.469.g59c65b2a67-goog
On Fri, Sep 13, 2024 at 03:50:00PM -0700, Jeff Xu wrote:
Even though the number of lines is large in these patches, its main intention is to test Pedro's in-place change (from can_modify_mm to can_modify_vma). Before this patch, the test had a common pattern: setup memory layout, seal the memory, perform a few mm-api steps, verify return code (not zero). Because of the nature of out-of-loop, it is sufficient to just verify the error code in a few cases.
With Pedro's in-loop change, the sealing check happens later in the stack, thus there are more things and scenarios to verify. And there were feedback to me during in-loop change that selftest should be extensive enough to discover all regressions. Even though this viewpoint is subject to debate. Since none would want to do it, I thought I would just do it.
So the Patch V3 1/5 is dedicated entirely to increasing the verification for existing scenarios, this including checking return code code, vma-size, etc after mm api return.
Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop change, we discovered a bug in unmap(), and unmap() is not atomic. This leads to 4/5(mmap), 5/5(mremap), which calls munmap(). In addition, I add scenarios to cover cross-multiple-vma cases.
The high-level goal of mseal test are two folds: 1> make sure sealing is working correctly under different scenarios, i.e. sealed mapping are not modified. 2> For unsealed memory, added mseal code doesn't regress on regular mm API.
The goal 2 is as important as 1, that is why tests usually are done in two phases, one with sealing, the other without.
That's vastly more detail than is in the changelogs for the actual patches (which are just a few lines each) or the cover letter of the series. I don't have the MM knowledge to assess the detail of what you're saying but I can't help but think that it'd help a lot with review if all this detail were part of the actual submission.
Hi Mark
On Wed, Sep 18, 2024 at 6:18 AM Mark Brown broonie@kernel.org wrote:
On Fri, Sep 13, 2024 at 03:50:00PM -0700, Jeff Xu wrote:
Even though the number of lines is large in these patches, its main intention is to test Pedro's in-place change (from can_modify_mm to can_modify_vma). Before this patch, the test had a common pattern: setup memory layout, seal the memory, perform a few mm-api steps, verify return code (not zero). Because of the nature of out-of-loop, it is sufficient to just verify the error code in a few cases.
With Pedro's in-loop change, the sealing check happens later in the stack, thus there are more things and scenarios to verify. And there were feedback to me during in-loop change that selftest should be extensive enough to discover all regressions. Even though this viewpoint is subject to debate. Since none would want to do it, I thought I would just do it.
So the Patch V3 1/5 is dedicated entirely to increasing the verification for existing scenarios, this including checking return code code, vma-size, etc after mm api return.
Patch V3 3/5 are for unmap(), during review of V2 of Pedro's in-loop change, we discovered a bug in unmap(), and unmap() is not atomic. This leads to 4/5(mmap), 5/5(mremap), which calls munmap(). In addition, I add scenarios to cover cross-multiple-vma cases.
The high-level goal of mseal test are two folds: 1> make sure sealing is working correctly under different scenarios, i.e. sealed mapping are not modified. 2> For unsealed memory, added mseal code doesn't regress on regular mm API.
The goal 2 is as important as 1, that is why tests usually are done in two phases, one with sealing, the other without.
That's vastly more detail than is in the changelogs for the actual patches (which are just a few lines each) or the cover letter of the series. I don't have the MM knowledge to assess the detail of what you're saying but I can't help but think that it'd help a lot with review if all this detail were part of the actual submission.
Agreed, will update and give more detail in the next version of the patch.
Thanks -Jeff
From: Jeff Xu jeffxu@chromium.org
Add sealing test to cover mremap for Expand/shrink/move across vmas.
Signed-off-by: Jeff Xu jeffxu@chromium.org --- tools/testing/selftests/mm/mseal_test.c | 202 +++++++++++++++++++++++- 1 file changed, 201 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c index 3516389034a7..fee655bbbf0a 100644 --- a/tools/testing/selftests/mm/mseal_test.c +++ b/tools/testing/selftests/mm/mseal_test.c @@ -2339,6 +2339,197 @@ static void test_seal_mmap_reuse_addr(bool seal) REPORT_TEST_PASS(); }
+static void test_seal_mremap_shrink_multiple_vmas(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + ret2 = sys_mremap(ptr, 12 * page_size, 6 * page_size, 0, 0); + if (seal) { + FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED); + FAIL_TEST_IF_FALSE(errno == EPERM); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + } else { + FAIL_TEST_IF_FALSE(ret2 == ptr); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 2 * page_size); + } + + REPORT_TEST_PASS(); +} + +static void test_seal_mremap_expand_multiple_vmas(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE); + FAIL_TEST_IF_FALSE(!ret); + + /* ummap last 4 pages. */ + ret = sys_munmap(ptr + 8 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + ret2 = sys_mremap(ptr, 8 * page_size, 12 * page_size, 0, 0); + FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + REPORT_TEST_PASS(); +} + +static void test_seal_mremap_move_expand_multiple_vmas(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + void *ptr2; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + setup_single_address(size, &ptr2); + FAIL_TEST_IF_FALSE(ptr2 != (void *)-1); + + ret = sys_munmap(ptr2, 12 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE); + FAIL_TEST_IF_FALSE(!ret); + + /* ummap last 4 pages. */ + ret = sys_munmap(ptr + 8 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* move and expand cross VMA boundary will fail */ + ret2 = sys_mremap(ptr, 8 * page_size, 10 * page_size, MREMAP_FIXED | MREMAP_MAYMOVE, ptr2); + FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + + REPORT_TEST_PASS(); +} + +static void test_seal_mremap_move_shrink_multiple_vmas(bool seal) +{ + void *ptr; + unsigned long page_size = getpagesize(); + unsigned long size = 12 * page_size; + int ret; + void *ret2; + int prot; + void *ptr2; + + setup_single_address(size, &ptr); + FAIL_TEST_IF_FALSE(ptr != (void *)-1); + + setup_single_address(size, &ptr2); + FAIL_TEST_IF_FALSE(ptr2 != (void *)-1); + + ret = sys_munmap(ptr2, 12 * page_size); + FAIL_TEST_IF_FALSE(!ret); + + ret = sys_mprotect(ptr + 4 * page_size, 4 * page_size, PROT_NONE); + FAIL_TEST_IF_FALSE(!ret); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0); + + if (seal) { + ret = sys_mseal(ptr + 4 * page_size, 4 * page_size); + FAIL_TEST_IF_FALSE(!ret); + } + + /* move and shrink cross VMA boundary is NOK */ + ret2 = sys_mremap(ptr, 12 * page_size, 8 * page_size, MREMAP_FIXED | MREMAP_MAYMOVE, ptr2); + FAIL_TEST_IF_FALSE(ret2 == (void *) MAP_FAILED); + + size = get_vma_size(ptr, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 4); + + size = get_vma_size(ptr + 4 * page_size, &prot); + FAIL_TEST_IF_FALSE(size == 4 * page_size); + FAIL_TEST_IF_FALSE(prot == 0); + + REPORT_TEST_PASS(); +} + int main(int argc, char **argv) { bool test_seal = seal_support(); @@ -2360,7 +2551,7 @@ int main(int argc, char **argv) if (!get_vma_size_supported()) ksft_exit_skip("get_vma_size not supported\n");
- ksft_set_plan(97); + ksft_set_plan(105);
test_seal_addseal(); test_seal_unmapped_start(); @@ -2481,5 +2672,14 @@ int main(int argc, char **argv) test_seal_mmap_reuse_addr(false); test_seal_mmap_reuse_addr(true);
+ test_seal_mremap_shrink_multiple_vmas(false); + test_seal_mremap_shrink_multiple_vmas(true); + test_seal_mremap_expand_multiple_vmas(false); + test_seal_mremap_expand_multiple_vmas(true); + test_seal_mremap_move_expand_multiple_vmas(false); + test_seal_mremap_move_expand_multiple_vmas(true); + test_seal_mremap_move_shrink_multiple_vmas(false); + test_seal_mremap_move_shrink_multiple_vmas(true); + ksft_finished(); }
On Fri, Aug 30, 2024 at 06:02:32PM GMT, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
This series increase the test coverage of mseal_test by:
Add check for vma_size, prot, and error code for existing tests. Add more testcases for madvise, munmap, mmap and mremap to cover sealing in different scenarios.
The increase test coverage hopefully help to prevent future regression. It doesn't change any existing mm api's semantics, i.e. it will pass on linux main and 6.10 branch.
This is a big improvement in detail, thanks.
Note: in order to pass this test in mm-unstable, mm-unstable must have Liam's fix on mmap [1]
[1] https://lore.kernel.org/linux-kselftest/vyllxuh5xbqmaoyl2mselebij5ox7cseekjc...
This is already in mm-unstable so this is redundant, and as Liam explained, his new v8 series will contain this fix too, and his old version will be unwound before new applied, so at no time will this be relevant.
History: V3:
- no-functional change, incooperate feedback from Pedro Falcato
V2:
- https://lore.kernel.org/linux-kselftest/20240829214352.963001-1-jeffxu@chrom...
- remove the mmap fix (Liam R. Howlett will fix it separately)
- Add cover letter (Lorenzo Stoakes)
I think I asked for more than this :)
- split the testcase for ease of review (Mark Brown)
V1:
Jeff Xu (5): selftests/mseal_test: Check vma_size, prot, error code. selftests/mseal: add sealed madvise type selftests/mseal: munmap across multiple vma ranges. selftests/mseal: add more tests for mmap selftests/mseal: add more tests for mremap
tools/testing/selftests/mm/mseal_test.c | 830 ++++++++++++++++++++++-- 1 file changed, 763 insertions(+), 67 deletions(-)
-- 2.46.0.469.g59c65b2a67-goog
Overall having checked one patch in the series, I am quite concerned that these tests are not testing what they suggest they do, are redundant, and I have found numerous problems line-by-line.
I've also encountered copy/pasted blocks, comparing PROT_xxx fields to magic numbers, and it generally feels really rushed.
I feel like it might be worth taking some time on the next respin to really think carefully about how these functions work, checking man pages and source, and getting some understanding of what it is we really need to assert about mseal() behaviour.
We're here to help you and want to be collaborative and help get mseal() into a good state, so I'd like to suggest taking your time on the next respin to really improve the quality and think carefully in each instance _what_ you are testing and _why_.
And don't be afraid to ask questions and discuss these things with us. We're happy to help!
Anyway, I am now off on a long weekend before my birthday, I wish you a great weekend and hope we can find a way to move forward constructively with this! :)
Thanks, Lorenzo
linux-kselftest-mirror@lists.linaro.org
-
Jeff Xu -
jeffxu＠chromium.org
-
Lorenzo Stoakes -
Mark Brown -
Pedro Falcato