Re: [RFC PATCH v3 1/2] xen: add libafl-qemu fuzzer support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2025 11:53, Volodymyr Babchuk wrote:
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -78,6 +78,7 @@ extra-y := symbols-dummy.o
>  obj-$(CONFIG_COVERAGE) += coverage/
>  obj-y += sched/
>  obj-$(CONFIG_UBSAN) += ubsan/
> +obj-$(CONFIG_FUZZER_LIBAFL_QEMU) += libafl-qemu.o

This ought to move up into the list of (mostly?) sorted object files.

> --- /dev/null
> +++ b/xen/common/libafl-qemu.c
> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> +  This file is based on libafl_qemu_impl.h, libafl_qemu_qemu_arch.h
> +  and libafl_qemu_defs.h from LibAFL project.
> +*/
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/kernel.h>
> +#include <xen/spinlock.h>
> +#include <xen/libafl-qemu.h>
> +#include <asm/libafl-qemu.h>
> +
> +/* Generates sync exit functions */
> +LIBAFL_DEFINE_FUNCTIONS(sync_exit, LIBAFL_SYNC_EXIT_OPCODE)
> +
> +    void libafl_qemu_end(enum LibaflQemuEndStatus status)
> +{
> +    _libafl_sync_exit_call1(LIBAFL_QEMU_COMMAND_END, status);
> +}
> +
> +void libafl_qemu_internal_error(void)
> +{
> +    _libafl_sync_exit_call0(LIBAFL_QEMU_COMMAND_INTERNAL_ERROR);
> +}
> +
> +void lqprintf(const char *fmt, ...)

At least this one looks as if it can be static. Anything which can be should
be made so.

> +{
> +    static DEFINE_SPINLOCK(lock);
> +    static char buffer[LIBAFL_QEMU_PRINTF_MAX_SIZE] = {0};
> +    va_list args;
> +    int res;
> +
> +    spin_lock(&lock);
> +
> +    va_start(args, fmt);
> +    res = vsnprintf(buffer, LIBAFL_QEMU_PRINTF_MAX_SIZE, fmt, args);
> +    va_end(args);
> +
> +    if ( res >= LIBAFL_QEMU_PRINTF_MAX_SIZE )
> +    {
> +        /* buffer is not big enough, either recompile the target with more */
> +        /* space or print less things */
> +        libafl_qemu_internal_error();
> +    }
> +
> +    _libafl_sync_exit_call2(LIBAFL_QEMU_COMMAND_LQPRINTF,
> +                            (libafl_word)buffer, res);
> +    spin_unlock(&lock);
> +}
> +
> +void libafl_qemu_trace_vaddr_range(libafl_word start,
> +                                   libafl_word end)
> +{
> +    _libafl_sync_exit_call2(LIBAFL_QEMU_COMMAND_VADDR_FILTER_ALLOW, start, 
> end);
> +}
> +
> +static int init_afl(void)
> +{
> +    vaddr_t xen_text_start = (vaddr_t)_stext;
> +    vaddr_t xen_text_end = (vaddr_t)_etext;
> +
> +    lqprintf("Telling AFL about code section: %lx - %lx\n", xen_text_start,
> +             xen_text_end);
> +
> +    libafl_qemu_trace_vaddr_range(xen_text_start, xen_text_end);
> +
> +    return 0;
> +}
> +
> +__initcall(init_afl);

Please omit the blank line ahead of the __initcall() if that immediately
follows the respective function.

> --- /dev/null
> +++ b/xen/include/xen/libafl-qemu.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: MIT */
> +#ifndef __XEN_LIBAFL_QEMU_H
> +#define __XEN_LIBAFL_QEMU_H
> +
> +#include <xen/stdint.h>
> +#define LIBAFL_QEMU_PRINTF_MAX_SIZE 4096
> +
> +#define LIBAFL_STRINGIFY(s) #s
> +#define XSTRINGIFY(s) LIBAFL_STRINGIFY(s)

We have STR() (and stringify()) - why would we need yet another macro?

> +#define LIBAFL_SYNC_EXIT_OPCODE 0x66f23a0f
> +
> +typedef enum LibaflQemuCommand
> +{
> +  LIBAFL_QEMU_COMMAND_START_VIRT = 0,
> +  LIBAFL_QEMU_COMMAND_START_PHYS = 1,
> +  LIBAFL_QEMU_COMMAND_INPUT_VIRT = 2,
> +  LIBAFL_QEMU_COMMAND_INPUT_PHYS = 3,
> +  LIBAFL_QEMU_COMMAND_END = 4,
> +  LIBAFL_QEMU_COMMAND_SAVE = 5,
> +  LIBAFL_QEMU_COMMAND_LOAD = 6,
> +  LIBAFL_QEMU_COMMAND_VERSION = 7,
> +  LIBAFL_QEMU_COMMAND_VADDR_FILTER_ALLOW = 8,
> +  LIBAFL_QEMU_COMMAND_INTERNAL_ERROR = 9,
> +  LIBAFL_QEMU_COMMAND_LQPRINTF = 10,
> +  LIBAFL_QEMU_COMMAND_TEST = 11,
> +} LibaflExit;
> +
> +typedef uint64_t libafl_word;

Looking at its uses, this rather wants to be unsigned long as it seems.

Jan