fix(api): check for empty range or overflow
For `FFA_MEM_PERM_GET` and `FFA_MEM_PERM_SET`, check for an empty range
(`page_count == 0`) or an overflow when calculating the end range.
Change-Id: I0c0b69728c996aa169d1f172a98e62ab5f80ffe9
Signed-off-by: Karl Meakin <karl.meakin@arm.com>
diff --git a/inc/hf/arch/std.h b/inc/hf/arch/std.h
index 2e29e21..ff606b2 100644
--- a/inc/hf/arch/std.h
+++ b/inc/hf/arch/std.h
@@ -38,13 +38,18 @@
#define align_down(v, a) __builtin_align_down((v), (a))
/*
- * Add operation together with checking whether the operation overflowed
- * The result is '*res',
- * return false on success and true on overflow
+ * Calculate the sum `a + b` and write the result to `*res`.
+ * Returns whether the operation overflowed.
*/
#define add_overflow(a, b, res) __builtin_add_overflow((a), (b), (res))
/*
+ * Calculate product `a * b` and write the result to `*res`.
+ * Returns whether the operation overflowed.
+ */
+#define mul_overflow(a, b, res) __builtin_mul_overflow((a), (b), (res))
+
+/*
* Round up a value to align with a given size and
* check whether overflow happens.
* The rounded value is '*res', return false on success and true on overflow.
diff --git a/src/api.c b/src/api.c
index de0e427..494cfef 100644
--- a/src/api.c
+++ b/src/api.c
@@ -4570,6 +4570,29 @@
return result;
}
+/*
+ * Calculate the end of the memory range (`base_addr + page_count * PAGE_SIZE`)
+ * and write the result to `*res`.
+ * Returns whether any of the intermediate operations overflowed.
+ */
+static bool api_memory_range_end(vaddr_t base_addr, uint32_t page_count,
+ vaddr_t *res)
+{
+ uint64_t range_size;
+ uintvaddr_t end_addr;
+
+ if (mul_overflow(page_count, PAGE_SIZE, &range_size)) {
+ return true;
+ }
+
+ if (add_overflow(va_addr(base_addr), range_size, &end_addr)) {
+ return true;
+ }
+
+ *res = va_init(end_addr);
+ return false;
+}
+
struct ffa_value api_ffa_mem_perm_get(vaddr_t base_addr, uint32_t page_count,
struct vcpu *current)
{
@@ -4584,9 +4607,21 @@
* granule size to ensure backwards compatability: v1.2 or earlier
* callers, who leave `arg2` as 0, will get the correct behaviour
* (querying a single page).
+ *
+ * Any overflow will be caught by the check against zero.
*/
page_count += 1;
- end_addr = va_add(base_addr, page_count * PAGE_SIZE);
+
+ /* Empty ranges should be disallowed, as should ranges that overflow */
+ if (page_count == 0) {
+ dlog_error("FFA_MEM_PERM_GET: page_count was zero\n");
+ return ffa_error(FFA_INVALID_PARAMETERS);
+ }
+
+ if (api_memory_range_end(base_addr, page_count, &end_addr)) {
+ dlog_error("FFA_MEM_PERM_GET: overflow calculating end_addr\n");
+ return ffa_error(FFA_INVALID_PARAMETERS);
+ }
if (!ffa_memory_is_mem_perm_get_valid(current)) {
dlog_error("FFA_MEM_PERM_GET: not allowed\n");
@@ -4667,7 +4702,7 @@
mm_mode_t original_mode;
mm_mode_t new_mode;
struct mpool local_page_pool;
- vaddr_t end_addr = va_add(base_addr, page_count * PAGE_SIZE);
+ vaddr_t end_addr;
if (!ffa_memory_is_mem_perm_set_valid(current)) {
dlog_error("FFA_MEM_PERM_SET: not allowed\n");
@@ -4688,6 +4723,17 @@
return ffa_error(FFA_INVALID_PARAMETERS);
}
+ /* Empty ranges should be disallowed, as should ranges that overflow */
+ if (page_count == 0) {
+ dlog_error("FFA_MEM_PERM_SET: page_count was zero\n");
+ return ffa_error(FFA_INVALID_PARAMETERS);
+ }
+
+ if (api_memory_range_end(base_addr, page_count, &end_addr)) {
+ dlog_error("FFA_MEM_PERM_SET: overflow calculating end_addr\n");
+ return ffa_error(FFA_INVALID_PARAMETERS);
+ }
+
switch (mem_perm) {
case FFA_MEM_PERM_RO:
new_mode = MM_MODE_R | MM_MODE_USER | MM_MODE_NG;
diff --git a/test/vmapi/primary_with_secondaries/services/arch/aarch64/el0/mem_permissions_test.c b/test/vmapi/primary_with_secondaries/services/arch/aarch64/el0/mem_permissions_test.c
index ff294f6..860c773 100644
--- a/test/vmapi/primary_with_secondaries/services/arch/aarch64/el0/mem_permissions_test.c
+++ b/test/vmapi/primary_with_secondaries/services/arch/aarch64/el0/mem_permissions_test.c
@@ -175,10 +175,6 @@
FFA_MEM_PERM_RW);
test_perm_get_range(data_end, image_end, image_end, FFA_MEM_PERM_RW);
- /* Check that permissions on an invalid address returns error. */
- expect_get_invalid((uintvaddr_t)text_begin + 1, 1);
- expect_get_invalid(0xDEADBEEF, 1);
- expect_get_invalid(0x0, 1);
expect_get_valid((uintvaddr_t)text_end - PAGE_SIZE, 2, 1,
FFA_MEM_PERM_RX);
@@ -188,6 +184,12 @@
/* Failure: unaligned base address */
expect_get_invalid((uintvaddr_t)text_begin + 1, 1);
+
+ /* Failure: empty range */
+ expect_get_invalid((uintvaddr_t)text_begin, 0);
+
+ /* Failure: overflow */
+ expect_get_invalid(UINT64_MAX, 1);
}
/**
@@ -208,22 +210,22 @@
expect_set_valid(base_va, 1, FFA_MEM_PERM_RW);
expect_get_full_valid(base_va, 1, FFA_MEM_PERM_RW);
- /* Ensure permission for invalid pages cannot be changed. */
- expect_set_invalid(0xDEADBEEF, 0x1000, FFA_MEM_PERM_RX);
- expect_set_invalid(0x0, 0x1000, FFA_MEM_PERM_RX);
+ /* Failure: unmapped base address */
+ expect_set_invalid(0x0, 1, FFA_MEM_PERM_RX);
+ expect_set_invalid(0xDEADBEEF, 1, FFA_MEM_PERM_RX);
- /**
- * Ensure permissions cannot be changed for an unaligned, but valid
- * address.
- */
+ /* Failure: unaligned base address */
expect_set_invalid(base_va + 1, 1, FFA_MEM_PERM_RX);
- /**
- * Ensure permissions cannot be changed for valid address that crosses
- * boundary into invalid address.
- */
+ /* Failure: base address is valid, but end address is invalid */
expect_set_invalid(base_va, 256, FFA_MEM_PERM_RX);
+ /* Failure: empty range */
+ expect_set_invalid((uintvaddr_t)text_begin, 0, FFA_MEM_PERM_RX);
+
+ /* Failure: overflow */
+ expect_set_invalid(UINT64_MAX, 1, FFA_MEM_PERM_RX);
+
/* Failure: invalid attributes */
expect_set_invalid(base_va, 1, 0x1);
}