Re: [RFC PATCH v2] xen: add libafl-qemu fuzzer support

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Julien,

Julien Grall <julien@xxxxxxx> writes:

[...]

>> diff --git a/xen/arch/arm/include/asm/libafl_qemu.h 
>> b/xen/arch/arm/include/asm/libafl_qemu.h
>> new file mode 100644
>> index 0000000000..b90cf48b9a
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/libafl_qemu.h
>> @@ -0,0 +1,54 @@
>> +#ifndef LIBAFL_QEMU_H
>> +#define LIBAFL_QEMU_H
>> +
>> +#include <xen/stdint.h>
>> +#include "libafl_qemu_defs.h"
>> +#define LIBAFL_QEMU_PRINTF_MAX_SIZE 4096
>
> Is this defined by libafl or an internal decision?

It is defined by libafl

>
> [...]
>
>> diff --git a/xen/arch/arm/include/asm/libafl_qemu_defs.h 
>> b/xen/arch/arm/include/asm/libafl_qemu_defs.h
>> new file mode 100644
>> index 0000000000..2866cadaac
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/libafl_qemu_defs.h
>> @@ -0,0 +1,37 @@
>
> Missing license. Also, is this file taken from somewhere?
>

I add MIT license, as libafl is dual licensed under Apache-2 and
MIT. This file is based on libafl_qemu [1]

>> +#ifndef LIBAFL_QEMU_DEFS
>> +#define LIBAFL_QEMU_DEFS
>> +
>> +#define LIBAFL_STRINGIFY(s) #s
>> +#define XSTRINGIFY(s) LIBAFL_STRINGIFY(s)
>> +
>> +#if __STDC_VERSION__ >= 201112L
>> +  #define STATIC_CHECKS                                   \
>> +    _Static_assert(sizeof(void *) <= sizeof(libafl_word), \
>> +                   "pointer type should not be larger and libafl_word");
>> +#else
>> +  #define STATIC_CHECKS
>> +#endif
>
> No-one seems to use STATIC_CHECKS? Is this intended?

I used this file as is... But I'll rework this part.

>> +
>> +#define LIBAFL_SYNC_EXIT_OPCODE 0x66f23a0f
>> +#define LIBAFL_BACKDOOR_OPCODE 0x44f23a0f
>
> Are the opcode valid for arm32? If not, they should be protected with
> #ifdef CONFIG_ARM_64.
>

It is valid even for x86_64. They use the same opcode for x86_64, arm,
aarch64 and riscv.

[...]

>> +
>> +#define LIBAFL_DEFINE_FUNCTIONS(name, opcode)                               
>> \
>> +    libafl_word _libafl_##name##_call0(     \
>> +            libafl_word action) {                                   \
>> +            libafl_word ret;                                        \
>> +            __asm__ volatile (                                      \
>> +                    "mov x0, %1\n"                                  \
>> +                    ".word " XSTRINGIFY(opcode) "\n"                \
>> +                    "mov %0, x0\n"                                  \
>> +                    : "=r"(ret)                                     \
>> +                    : "r"(action)                                   \
>> +                    : "x0"                                          \
>
> Can we store the action directly in x0 (same for the other argunments
> below)? This would avoid to clobber two registers (See smccc.h as an
> example).

Yes, this part bothers me also. I'll try to rework it to be more
efficient.


[...]

>> +
>> +libafl_word libafl_qemu_start_virt(void       *buf_vaddr,
>> +                                            libafl_word max_len) {
>
> What coding style is this file meant to use?

Well, LibAFL people is very lax in their coding style. I copied this
file as is, but probably it should be tidied up and minimized.


[...]

>> +void lqprintf(const char *fmt, ...) {
>
> I am not sure I understand the value of lqprinf(). Why can't we use
> the console? When is this meant to be used?

This is alternative way to output something. It skips all the
abstractions around console and outputs straight to stdout. At least,
this is a nice way to check that communication with the fuzzer is
working.

>> +  va_list args;
>> +  int res;
>> +  va_start(args, fmt);
>> +  res = vsnprintf(_lqprintf_buffer, LIBAFL_QEMU_PRINTF_MAX_SIZE, fmt, args);
>> +  va_end(args);
>
> What if lqprintf() is called concurrently?
>

I'll add a spinlock.

[...]

>>     void call_psci_cpu_off(void)
>>   {
>> +#ifdef CONFIG_LIBAFL_QEMU_FUZZER_PASS_BLOCKING
>> +    libafl_qemu_end(LIBAFL_QEMU_END_OK);
>> +#endif
>
> I am a bit confused with this call. For a first, this cannot be
> reached from a VM (or even dom0). Then, even if it is reached,
> shouldn't we allow the test continue while other pCPUs are running?

Yes, looks like this particular call is not accessible by a dom0. I'll
remove it.

> That said, the call to QEMU is not PSCI related. So shouldn't this be
> called from the callers (same applies to all the changes in PSCI)?

Purpose of fuzzing to cover as much code paths as possible. So it is
natural to put this as late as possible. I also reworked changes to
sched/core.c in accordance to this. I.e. moved fuzzer_on_block() calls
as late as possible.

>
>> +
>>       if ( psci_ver > PSCI_VERSION(0, 1) )
>>       {
>>           struct arm_smccc_res res;
>> @@ -62,12 +67,20 @@ void call_psci_cpu_off(void)
>>     void call_psci_system_off(void)
>>   {
>> +#ifdef CONFIG_LIBAFL_QEMU_FUZZER_PASS_BLOCKING
>> +    libafl_qemu_end(LIBAFL_QEMU_END_OK);
>> +#endif
>> +
>>       if ( psci_ver > PSCI_VERSION(0, 1) )
>>           arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_OFF, NULL);
>>   }
>>     void call_psci_system_reset(void)
>>   {
>> +#ifdef CONFIG_LIBAFL_QEMU_FUZZER_PASS_BLOCKING
>> +    libafl_qemu_end(LIBAFL_QEMU_END_OK);
>> +#endif
>> +
>>       if ( psci_ver > PSCI_VERSION(0, 1) )
>>           arm_smccc_smc(PSCI_0_2_FN32_SYSTEM_RESET, NULL);
>>   }
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 9043414290..55eb132568 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -47,6 +47,10 @@
>>   #define pv_shim false
>>   #endif
>>   +#ifdef CONFIG_LIBAFL_QEMU_FUZZER
>
> This Kconfig is only defined on Arm but you are using in common
> code. Even if this can't be supported right now, shouldn't this be
> defined in common code?

Yes, I am going to move it to the common code, but will it be fine to
have "depends on ARM_64" in the global ./Kconfig.debug for a time being?

>
>> +#include <asm/libafl_qemu.h>
>> +#endif
>> +
>>   /* opt_sched: scheduler - default to configured value */
>>   static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT;
>>   string_param("sched", opt_sched);
>> @@ -1452,6 +1456,10 @@ static long do_poll(const struct sched_poll 
>> *sched_poll)
>>       if ( !guest_handle_okay(sched_poll->ports, sched_poll->nr_ports) )
>>           return -EFAULT;
>>   +#ifdef CONFIG_LIBAFL_QEMU_FUZZER_PASS_BLOCKING
>> +    libafl_qemu_end(LIBAFL_QEMU_END_OK);
>> +#endif
>
> I think this and all the changes in sched/core need a comment
> explaning why we want to stop the fuzzing.

I introduced fuzzer_on_block() function and put the following comment
for it:

/*
 * Conditional success
 *
 * Sometimes a fuzzer might make Xen to do something that prevents
 * from returning to the caller: reboot or turn off the machine, block
 * calling vCPU, crash a domain, etc. Depending on fuzzing goal this
 * may be a valid behavior, but as control is not returned to the
 * fuzzing harness, it can't tell the fuzzer about success, so we need
 * to do this ourselves.
 */

Will it be enough? Or do you want to have a comment before each call to 
fuzzer_on_block()?


[1] 
https://github.com/AFLplusplus/LibAFL/blob/main/libafl_qemu/runtime/libafl_qemu_defs.h

-- 
WBR, Volodymyr