On 3/18/19 3:48 PM, Jan Kiszka wrote:
> On 03.01.19 19:06, Ralf Ramsauer wrote:
>> Crash a cell if it calls an unhandled SMC functions.
>>
>> There are two reason why to do this:
>> - Not all SMC calls have return values. If the hypervisor returns with
>> UNHANDLED, the guest may silently fail as it takes wrong
>> assumptions. (This is what already happened to us)
>> - A guest may only invoke SMC functions which it has discovered
>> before. If there are new functions that we might have to implement
>> in future, the crash will lead us the way.
>>
>> Note that the default handler crashes the cell in case of
>> ARCH_SMCCC_WORKAROUND_1, as the default handler path will only be taken
>> in case of default interrupt vectors, where the workaround is not
>> available.
>>
>> Signed-off-by: Ralf Ramsauer <[email protected]>
>> ---
>> hypervisor/arch/arm-common/smccc.c | 26 +++++++++++++++-----------
>> 1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/hypervisor/arch/arm-common/smccc.c
>> b/hypervisor/arch/arm-common/smccc.c
>> index 4d0392f9..58392e9b 100644
>> --- a/hypervisor/arch/arm-common/smccc.c
>> +++ b/hypervisor/arch/arm-common/smccc.c
>> @@ -62,32 +62,37 @@ static inline long handle_arch_features(u32 id)
>> }
>> }
>> -static long handle_arch(struct trap_context *ctx)
>> +static enum trap_return handle_arch(struct trap_context *ctx)
>> {
>> - u32 function_id = ctx->regs[0];
>> + unsigned long *ret = &ctx->regs[0];
>> - switch (function_id) {
>> + switch (*ret) {
>> case SMCCC_VERSION:
>> - return ARM_SMCCC_VERSION_1_1;
>> + *ret = ARM_SMCCC_VERSION_1_1;
>> + break;
>> case SMCCC_ARCH_FEATURES:
>> - return handle_arch_features(ctx->regs[1]);
>> + *ret = handle_arch_features(ctx->regs[1]);
>> + break;
>> default:
>> - return ARM_SMCCC_NOT_SUPPORTED;
>> + panic_printk("Unhandled SMC arch trap %lx\n", *ret);
>> + return TRAP_UNHANDLED;
>> }
>> +
>> + return TRAP_HANDLED;
>> }
>> enum trap_return handle_smc(struct trap_context *ctx)
>> {
>> unsigned long *regs = ctx->regs;
>> + enum trap_return ret = TRAP_HANDLED;
>> u32 *stats = this_cpu_public()->stats;
>> switch (SMCCC_GET_OWNER(regs[0])) {
>> case ARM_SMCCC_OWNER_ARCH:
>> stats[JAILHOUSE_CPU_STAT_VMEXITS_SMCCC]++;
>> - regs[0] = handle_arch(ctx);
>> - break;
>> + ret = handle_arch(ctx);
>
> Coverity remarks, now that it's speaking again, that this looks like an
> unwanted fall-through regression...
Yikes. If I remember correctly, this used to be a `return
handle_arch(ctx)` in a previous patch. This is how this sneaked in
without a break...
Will provide a patch soon.
Hm, I wonder what kind of side effects this might have had. Probably
none for the root cell (as it won't issue such kind of requests when
running), and NOT_SUPPORTED as an answer for any OWNER_ARCH request of
non-root cells.
Ralf
>
> Jan
>
>> case ARM_SMCCC_OWNER_SIP:
>> stats[JAILHOUSE_CPU_STAT_VMEXITS_SMCCC]++;
>> @@ -99,11 +104,10 @@ enum trap_return handle_smc(struct trap_context
>> *ctx)
>> break;
>> default:
>> - return TRAP_UNHANDLED;
>> -
>> + ret = TRAP_UNHANDLED;
>> }
>> arch_skip_instruction(ctx);
>> - return TRAP_HANDLED;
>> + return ret;
>> }
>>
>
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
