SMC whitelist from the manifest.
This works for a small number of SMCs. `smc_whitelist` is a list of the
SMCs a VM is allowed to make. `smc_whitelist_permissive` can be set to
allow SMCs through even if they are not whitelisted (for development and
debug).
Bug: 132421503
Change-Id: I64b243d551da35f7625368a72a5a3980d63752f9
diff --git a/src/arch/aarch64/hypervisor/handler.c b/src/arch/aarch64/hypervisor/handler.c
index acbe562..00ba80b 100644
--- a/src/arch/aarch64/hypervisor/handler.c
+++ b/src/arch/aarch64/hypervisor/handler.c
@@ -256,51 +256,58 @@
write_msr(hcr_el2, hcr_el2);
}
-static bool smc_check_client_privileges(const struct vcpu *vcpu)
+/**
+ * Checks whether to block an SMC being forwarded from a VM.
+ */
+static bool smc_is_blocked(const struct vm *vm, uint32_t func)
{
- (void)vcpu; /*UNUSED*/
+ bool block_by_default = !vm->smc_whitelist.permissive;
- /*
- * TODO(b/132421503): Check for privileges based on manifest.
- * Currently returns false, which maintains existing behavior.
- */
+ for (size_t i = 0; i < vm->smc_whitelist.smc_count; ++i) {
+ if (func == vm->smc_whitelist.smcs[i]) {
+ return false;
+ }
+ }
- return false;
+ dlog("SMC %#010x attempted from VM %d, blocked=%d\n", func, vm->id,
+ block_by_default);
+
+ /* Access is still allowed in permissive mode. */
+ return block_by_default;
}
/**
- * Applies SMC access control according to manifest.
- * Forwards the call if access is granted.
- * Returns true if call is forwarded.
+ * Applies SMC access control according to manifest and forwards the call if
+ * access is granted.
*/
-static bool smc_forwarder(const struct vcpu *vcpu, struct smc_result *ret)
+static void smc_forwarder(const struct vcpu *vcpu, struct smc_result *ret)
{
uint32_t func = vcpu->regs.r[0];
- /* TODO(b/132421503): obtain vmid according to new scheme. */
uint32_t client_id = vcpu->vm->id;
+ uintreg_t arg7;
+
+ if (smc_is_blocked(vcpu->vm, func)) {
+ ret->res0 = SMCCC_ERROR_UNKNOWN;
+ return;
+ }
+
/*
* Set the Client ID but keep the existing Secure OS ID and anything
* else (currently unspecified) that the client may have passed in the
* upper bits.
*/
- uintreg_t arg7 = client_id | (vcpu->regs.r[7] & ~CLIENT_ID_MASK);
+ arg7 = client_id | (vcpu->regs.r[7] & ~CLIENT_ID_MASK);
+ *ret = smc_forward(func, vcpu->regs.r[1], vcpu->regs.r[2],
+ vcpu->regs.r[3], vcpu->regs.r[4], vcpu->regs.r[5],
+ vcpu->regs.r[6], arg7);
- if (smc_check_client_privileges(vcpu)) {
- *ret = smc_forward(func, vcpu->regs.r[1], vcpu->regs.r[2],
- vcpu->regs.r[3], vcpu->regs.r[4],
- vcpu->regs.r[5], vcpu->regs.r[6], arg7);
- /*
- * Preserve the value passed by the caller, rather than the
- * client_id we generated. Note that this would also overwrite
- * any return value that may be in x7, but the SMCs that we are
- * forwarding are legacy calls from before SMCCC 1.2 so won't
- * have more than 4 return values anyway.
- */
- ret->res7 = vcpu->regs.r[7];
- return true;
- }
-
- return false;
+ /*
+ * Preserve the value passed by the caller, rather than the client_id we
+ * generated. Note that this would also overwrite any return value that
+ * may be in x7, but the SMCs that we are forwarding are legacy calls
+ * from before SMCCC 1.2 so won't have more than 4 return values anyway.
+ */
+ ret->res7 = vcpu->regs.r[7];
}
static bool spci_handler(struct spci_value *args, struct vcpu **next)
@@ -362,25 +369,23 @@
/**
* Processes SMC instruction calls.
*/
-static bool smc_handler(struct vcpu *vcpu, struct smc_result *ret,
+static void smc_handler(struct vcpu *vcpu, struct smc_result *ret,
struct vcpu **next)
{
uint32_t func = vcpu->regs.r[0];
if (psci_handler(vcpu, func, vcpu->regs.r[1], vcpu->regs.r[2],
vcpu->regs.r[3], &ret->res0, next)) {
- /* SMC PSCI calls are processed by the PSCI handler. */
- return true;
+ return;
}
switch (func & ~SMCCC_CONVENTION_MASK) {
case HF_DEBUG_LOG:
- api_debug_log(vcpu->regs.r[1], vcpu);
- return true;
+ ret->res0 = api_debug_log(vcpu->regs.r[1], vcpu);
+ return;
}
- /* Remaining SMC calls need to be forwarded. */
- return smc_forwarder(vcpu, ret);
+ smc_forwarder(vcpu, ret);
}
struct vcpu *hvc_handler(struct vcpu *vcpu)
@@ -583,17 +588,13 @@
case 0x17: /* EC = 010111, SMC instruction. */ {
uintreg_t smc_pc = vcpu->regs.pc;
+ struct vcpu *next = NULL;
struct smc_result ret = {.res4 = vcpu->regs.r[4],
.res5 = vcpu->regs.r[5],
.res6 = vcpu->regs.r[6],
.res7 = vcpu->regs.r[7]};
- struct vcpu *next = NULL;
- if (!smc_handler(vcpu, &ret, &next)) {
- /* TODO(b/132421503): handle SMC forward rejection */
- dlog("Unsupported SMC call: %#x\n", vcpu->regs.r[0]);
- ret.res0 = SMCCC_ERROR_UNKNOWN;
- }
+ smc_handler(vcpu, &ret, &next);
/* Skip the SMC instruction. */
vcpu->regs.pc = smc_pc + GET_NEXT_PC_INC(esr);
diff --git a/src/load.c b/src/load.c
index 8df4fe2..5c7fa2a 100644
--- a/src/load.c
+++ b/src/load.c
@@ -92,10 +92,21 @@
}
/**
+ * Performs VM loading activities that are common between the primary and
+ * secondaries.
+ */
+static bool load_common(const struct manifest_vm *manifest_vm, struct vm *vm)
+{
+ vm->smc_whitelist = manifest_vm->smc_whitelist;
+
+ return true;
+}
+
+/**
* Loads the primary VM.
*/
static bool load_primary(struct mm_stage1_locked stage1_locked,
- const struct manifest *manifest,
+ const struct manifest_vm *manifest_vm,
const struct memiter *cpio,
const struct boot_params *params, struct mpool *ppool)
{
@@ -110,8 +121,8 @@
*/
paddr_t primary_end = pa_add(primary_begin, 0x8000000);
- if (!load_kernel(stage1_locked, primary_begin, primary_end,
- &manifest->vm[HF_PRIMARY_VM_INDEX], cpio, ppool)) {
+ if (!load_kernel(stage1_locked, primary_begin, primary_end, manifest_vm,
+ cpio, ppool)) {
dlog("Unable to load primary kernel.");
return false;
}
@@ -126,6 +137,10 @@
return false;
}
+ if (!load_common(manifest_vm, vm)) {
+ return false;
+ }
+
/*
* Map 1TB of address space as device memory to, most likely, make all
* devices available to the primary VM.
@@ -186,6 +201,10 @@
return false;
}
+ if (!load_common(manifest_vm, vm)) {
+ return false;
+ }
+
/* Grant the VM access to the memory. */
if (!mm_vm_identity_map(&vm->ptable, mem_begin, mem_end,
MM_MODE_R | MM_MODE_W | MM_MODE_X,
@@ -292,7 +311,8 @@
struct mem_range mem_ranges_available[MAX_MEM_RANGES];
size_t i;
- if (!load_primary(stage1_locked, manifest, cpio, params, ppool)) {
+ if (!load_primary(stage1_locked, &manifest->vm[HF_PRIMARY_VM_INDEX],
+ cpio, params, ppool)) {
dlog("Unable to load primary VM.\n");
return false;
}
diff --git a/src/manifest.c b/src/manifest.c
index 222716f..1933d10 100644
--- a/src/manifest.c
+++ b/src/manifest.c
@@ -18,6 +18,7 @@
#include "hf/addr.h"
#include "hf/check.h"
+#include "hf/dlog.h"
#include "hf/fdt.h"
#include "hf/static_assert.h"
#include "hf/std.h"
@@ -53,6 +54,24 @@
return ptr;
}
+/**
+ * Read a boolean property: true if present; false if not. The value of the
+ * property is ignored.
+ *
+ * This is the convention used by Linux but beware of things like the following
+ * that will actually be considered as `true`.
+ *
+ * true-property0 = <0>;
+ * true-property1 = "false";
+ */
+static bool read_bool(const struct fdt_node *node, const char *property)
+{
+ const char *data;
+ uint32_t size;
+
+ return fdt_read_property(node, property, &data, &size);
+}
+
static enum manifest_return_code read_string(const struct fdt_node *node,
const char *property,
struct string *out)
@@ -121,6 +140,30 @@
return MANIFEST_SUCCESS;
}
+struct uint32list_iter {
+ struct memiter mem_it;
+};
+
+static enum manifest_return_code read_optional_uint32list(
+ const struct fdt_node *node, const char *property,
+ struct uint32list_iter *out)
+{
+ const char *data;
+ uint32_t size;
+
+ if (!fdt_read_property(node, property, &data, &size)) {
+ memiter_init(&out->mem_it, NULL, 0);
+ return MANIFEST_SUCCESS;
+ }
+
+ if ((size % sizeof(uint32_t)) != 0) {
+ return MANIFEST_ERROR_MALFORMED_INTEGER_LIST;
+ }
+
+ memiter_init(&out->mem_it, data, size);
+ return MANIFEST_SUCCESS;
+}
+
/**
* Represents the value of property whose type is a list of strings. These are
* encoded as one contiguous byte buffer with NULL-separated entries.
@@ -152,6 +195,26 @@
return MANIFEST_SUCCESS;
}
+static bool uint32list_has_next(const struct uint32list_iter *list)
+{
+ return memiter_size(&list->mem_it) > 0;
+}
+
+static uint32_t uint32list_get_next(struct uint32list_iter *list)
+{
+ const char *mem_base = memiter_base(&list->mem_it);
+ uint64_t num;
+
+ CHECK(uint32list_has_next(list));
+
+ if (!fdt_parse_number(mem_base, sizeof(uint32_t), &num)) {
+ return MANIFEST_ERROR_MALFORMED_INTEGER;
+ }
+
+ memiter_advance(&list->mem_it, sizeof(uint32_t));
+ return num;
+}
+
static bool stringlist_has_next(const struct stringlist_iter *list)
{
return memiter_size(&list->mem_it) > 0;
@@ -206,9 +269,26 @@
struct manifest_vm *vm,
spci_vm_id_t vm_id)
{
+ struct uint32list_iter smcs;
+
TRY(read_string(node, "debug_name", &vm->debug_name));
TRY(read_optional_string(node, "kernel_filename",
&vm->kernel_filename));
+
+ TRY(read_optional_uint32list(node, "smc_whitelist", &smcs));
+ while (uint32list_has_next(&smcs) &&
+ vm->smc_whitelist.smc_count < MAX_SMCS) {
+ vm->smc_whitelist.smcs[vm->smc_whitelist.smc_count++] =
+ uint32list_get_next(&smcs);
+ }
+
+ if (uint32list_has_next(&smcs)) {
+ dlog("%s SMC whitelist too long.\n", vm->debug_name);
+ }
+
+ vm->smc_whitelist.permissive =
+ read_bool(node, "smc_whitelist_permissive");
+
if (vm_id == HF_PRIMARY_VM_ID) {
TRY(read_optional_string(node, "ramdisk_filename",
&vm->primary.ramdisk_filename));
@@ -314,6 +394,8 @@
return "Malformed integer property";
case MANIFEST_ERROR_INTEGER_OVERFLOW:
return "Integer overflow";
+ case MANIFEST_ERROR_MALFORMED_INTEGER_LIST:
+ return "Malformed integer list property";
}
panic("Unexpected manifest return code.");
diff --git a/src/manifest_test.cc b/src/manifest_test.cc
index 902cf21..ec38a56 100644
--- a/src/manifest_test.cc
+++ b/src/manifest_test.cc
@@ -16,6 +16,7 @@
#include <array>
#include <cstdio>
+#include <span>
#include <sstream>
#include <gmock/gmock.h>
@@ -26,7 +27,9 @@
namespace
{
+using ::testing::ElementsAre;
using ::testing::Eq;
+using ::testing::IsEmpty;
using ::testing::NotNull;
template <typename T>
@@ -120,7 +123,7 @@
StartChild("/");
}
- std::vector<char> Build()
+ std::vector<char> Build(bool dump = false)
{
const char *program = "./build/image/dtc.py";
const char *dtc_args[] = {program, "compile", NULL};
@@ -129,10 +132,19 @@
/* Finish root node. */
EndChild();
+ if (dump) {
+ Dump();
+ }
+
exec(program, dtc_args, dts_.str(), &dtc_stdout);
return dtc_stdout;
}
+ void Dump()
+ {
+ std::cerr << dts_.str() << std::endl;
+ }
+
ManifestDtBuilder &StartChild(const std::string_view &name)
{
dts_ << name << " {" << std::endl;
@@ -166,16 +178,33 @@
return StringProperty("ramdisk_filename", value);
}
- ManifestDtBuilder &VcpuCount(uint64_t value)
+ ManifestDtBuilder &VcpuCount(uint32_t value)
{
return IntegerProperty("vcpu_count", value);
}
- ManifestDtBuilder &MemSize(uint64_t value)
+ ManifestDtBuilder &MemSize(uint32_t value)
{
return IntegerProperty("mem_size", value);
}
+ ManifestDtBuilder &SmcWhitelist(const std::vector<uint32_t> &value)
+ {
+ return IntegerListProperty("smc_whitelist", value);
+ }
+
+ ManifestDtBuilder &SmcWhitelistPermissive()
+ {
+ return BooleanProperty("smc_whitelist_permissive");
+ }
+
+ ManifestDtBuilder &Property(const std::string_view &name,
+ const std::string_view &value)
+ {
+ dts_ << name << " = " << value << ";" << std::endl;
+ return *this;
+ }
+
private:
ManifestDtBuilder &StringProperty(const std::string_view &name,
const std::string_view &value)
@@ -204,12 +233,29 @@
}
ManifestDtBuilder &IntegerProperty(const std::string_view &name,
- uint64_t value)
+ uint32_t value)
{
dts_ << name << " = <" << value << ">;" << std::endl;
return *this;
}
+ ManifestDtBuilder &IntegerListProperty(
+ const std::string_view &name,
+ const std::vector<uint32_t> &value)
+ {
+ dts_ << name << " = < ";
+ for (const uint32_t entry : value) {
+ dts_ << entry << " ";
+ }
+ dts_ << ">;" << std::endl;
+ return *this;
+ }
+
+ ManifestDtBuilder &BooleanProperty(const std::string_view &name)
+ {
+ return IntegerProperty(name, 1);
+ }
+
std::stringstream dts_;
};
@@ -365,7 +411,7 @@
ASSERT_EQ(manifest_init(&m, &fdt_root), MANIFEST_ERROR_RESERVED_VM_ID);
}
-static std::vector<char> gen_vcpu_count_limit_dtb(uint64_t vcpu_count)
+static std::vector<char> gen_vcpu_count_limit_dtb(uint32_t vcpu_count)
{
/* clang-format off */
return ManifestDtBuilder()
@@ -426,6 +472,51 @@
ASSERT_STREQ(string_data(&m.vm[0].primary.ramdisk_filename), "");
}
+TEST(manifest, valid_true_booleans)
+{
+ struct manifest m;
+ struct manifest_vm *vm;
+ struct fdt_node fdt_root;
+
+ /* clang-format off */
+ std::vector<char> dtb = ManifestDtBuilder()
+ .StartChild("hypervisor")
+ .Compatible()
+ .StartChild("vm1")
+ .DebugName("primary_vm")
+ .Property("smc_whitelist_permissive", "\"false\"")
+ .EndChild()
+ .StartChild("vm2")
+ .DebugName("first_secondary_vm")
+ .VcpuCount(42)
+ .MemSize(12345)
+ .Property("smc_whitelist_permissive", "<0>")
+ .EndChild()
+ .StartChild("vm3")
+ .DebugName("second_secondary_vm")
+ .VcpuCount(43)
+ .MemSize(0x12345)
+ .Property("smc_whitelist_permissive", "\"true\"")
+ .EndChild()
+ .EndChild()
+ .Build();
+ /* clang-format on */
+
+ ASSERT_TRUE(get_fdt_root(dtb, &fdt_root));
+
+ ASSERT_EQ(manifest_init(&m, &fdt_root), MANIFEST_SUCCESS);
+ ASSERT_EQ(m.vm_count, 3);
+
+ vm = &m.vm[0];
+ ASSERT_TRUE(vm->smc_whitelist.permissive);
+
+ vm = &m.vm[1];
+ ASSERT_TRUE(vm->smc_whitelist.permissive);
+
+ vm = &m.vm[2];
+ ASSERT_TRUE(vm->smc_whitelist.permissive);
+}
+
TEST(manifest, valid)
{
struct manifest m;
@@ -440,6 +531,7 @@
.DebugName("primary_vm")
.KernelFilename("primary_kernel")
.RamdiskFilename("primary_ramdisk")
+ .SmcWhitelist({0x32000000, 0x33001111})
.EndChild()
.StartChild("vm3")
.DebugName("second_secondary_vm")
@@ -451,6 +543,8 @@
.DebugName("first_secondary_vm")
.VcpuCount(42)
.MemSize(12345)
+ .SmcWhitelist({0x04000000, 0x30002222, 0x31445566})
+ .SmcWhitelistPermissive()
.EndChild()
.EndChild()
.Build();
@@ -466,12 +560,20 @@
ASSERT_STREQ(string_data(&vm->kernel_filename), "primary_kernel");
ASSERT_STREQ(string_data(&vm->primary.ramdisk_filename),
"primary_ramdisk");
+ ASSERT_THAT(
+ std::span(vm->smc_whitelist.smcs, vm->smc_whitelist.smc_count),
+ ElementsAre(0x32000000, 0x33001111));
+ ASSERT_FALSE(vm->smc_whitelist.permissive);
vm = &m.vm[1];
ASSERT_STREQ(string_data(&vm->debug_name), "first_secondary_vm");
ASSERT_STREQ(string_data(&vm->kernel_filename), "");
ASSERT_EQ(vm->secondary.vcpu_count, 42);
ASSERT_EQ(vm->secondary.mem_size, 12345);
+ ASSERT_THAT(
+ std::span(vm->smc_whitelist.smcs, vm->smc_whitelist.smc_count),
+ ElementsAre(0x04000000, 0x30002222, 0x31445566));
+ ASSERT_TRUE(vm->smc_whitelist.permissive);
vm = &m.vm[2];
ASSERT_STREQ(string_data(&vm->debug_name), "second_secondary_vm");
@@ -479,6 +581,10 @@
"second_secondary_kernel");
ASSERT_EQ(vm->secondary.vcpu_count, 43);
ASSERT_EQ(vm->secondary.mem_size, 0x12345);
+ ASSERT_THAT(
+ std::span(vm->smc_whitelist.smcs, vm->smc_whitelist.smc_count),
+ IsEmpty());
+ ASSERT_FALSE(vm->smc_whitelist.permissive);
}
} /* namespace */