On Wed, Mar 1, 2023 at 12:07 AM Shaoqin Huang <shahuang@xxxxxxxxxx> wrote:
>
>
>
> On 3/1/23 13:34, Aaron Lewis wrote:
> > Add more flexibility to guest debugging and testing by adding
> > GUEST_PRINTF() and GUEST_ASSERT_FMT() to the ucall framework.
> >
> > A buffer to hold the formatted string was added to the ucall struct.
> > That allows the guest/host to avoid the problem of passing an
> > arbitrary number of parameters between themselves when resolving the
> > string. Instead, the string is resolved in the guest then passed
> > back to the host to be logged.
> >
> > The formatted buffer is set to 1024 bytes which increases the size
> > of the ucall struct. As a result, this will increase the number of
> > pages requested for the guest.
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> > ---
> > .../selftests/kvm/include/ucall_common.h | 19 +++++++++++++++++++
> > .../testing/selftests/kvm/lib/ucall_common.c | 19 +++++++++++++++++++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h
> > index 0b1fde23729b..2a4400b6761a 100644
> > --- a/tools/testing/selftests/kvm/include/ucall_common.h
> > +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> > @@ -13,15 +13,18 @@ enum {
> > UCALL_NONE,
> > UCALL_SYNC,
> > UCALL_ABORT,
> > + UCALL_PRINTF,
> > UCALL_DONE,
> > UCALL_UNHANDLED,
> > };
> >
> > #define UCALL_MAX_ARGS 7
> > +#define UCALL_BUFFER_LEN 1024
> >
> > struct ucall {
> > uint64_t cmd;
> > uint64_t args[UCALL_MAX_ARGS];
> > + char buffer[UCALL_BUFFER_LEN];
> Hi Aaron,
>
> A simple question, what if someone print too long in guest which exceed
> the UCALL_BUFFER_LEN, it seems buffer overflow will happen since
> vsprintf will not check the buffer length.
>
> Just in case, someone may don't know the limit and print too long.
>
> Thanks,
> Shaoqin
> >
In the followup I can check the length of the string written in
ucall_fmt() and return an overflow assert to the host instead if one
is detected.
> > /* Host virtual address of this struct. */
> > struct ucall *hva;
> > @@ -32,6 +35,7 @@ void ucall_arch_do_ucall(vm_vaddr_t uc);
> > void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu);
> >
> > void ucall(uint64_t cmd, int nargs, ...);
> > +void ucall_fmt(uint64_t cmd, const char *fmt, ...);
> > uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc);
> > void ucall_init(struct kvm_vm *vm, vm_paddr_t mmio_gpa);
> > int ucall_header_size(void);
> > @@ -47,6 +51,7 @@ int ucall_header_size(void);
> > #define GUEST_SYNC_ARGS(stage, arg1, arg2, arg3, arg4) \
> > ucall(UCALL_SYNC, 6, "hello", stage, arg1, arg2, arg3, arg4)
> > #define GUEST_SYNC(stage) ucall(UCALL_SYNC, 2, "hello", stage)
> > +#define GUEST_PRINTF(fmt, _args...) ucall_fmt(UCALL_PRINTF, fmt, ##_args)
> > #define GUEST_DONE() ucall(UCALL_DONE, 0)
> >
> > enum guest_assert_builtin_args {
> > @@ -56,6 +61,18 @@ enum guest_assert_builtin_args {
> > GUEST_ASSERT_BUILTIN_NARGS
> > };
> >
> > +#define __GUEST_ASSERT_FMT(_condition, _condstr, format, _args...) \
> > +do { \
> > + if (!(_condition)) \
> > + ucall_fmt(UCALL_ABORT, \
> > + "Failed guest assert: " _condstr \
> > + " at %s:%ld\n " format, \
> > + __FILE__, __LINE__, ##_args); \
> > +} while (0)
> > +
> > +#define GUEST_ASSERT_FMT(_condition, format, _args...) \
> > + __GUEST_ASSERT_FMT(_condition, #_condition, format, ##_args)
> > +
> > #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) \
> > do { \
> > if (!(_condition)) \
> > @@ -81,6 +98,8 @@ do { \
> >
> > #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, b)
> >
> > +#define REPORT_GUEST_ASSERT_FMT(_ucall) TEST_FAIL("%s", _ucall.buffer)
> > +
> > #define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \
> > TEST_FAIL("%s at %s:%ld\n" fmt, \
> > (const char *)(_ucall).args[GUEST_ERROR_STRING], \
> > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > index b6a75858fe0d..92ebc5db1c41 100644
> > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > @@ -54,7 +54,9 @@ static struct ucall *ucall_alloc(void)
> > for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> > if (!test_and_set_bit(i, ucall_pool->in_use)) {
> > uc = &ucall_pool->ucalls[i];
> > + uc->cmd = UCALL_NONE;
> > memset(uc->args, 0, sizeof(uc->args));
> > + memset(uc->buffer, 0, sizeof(uc->buffer));
> > return uc;
> > }
> > }
> > @@ -75,6 +77,23 @@ static void ucall_free(struct ucall *uc)
> > clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
> > }
> >
> > +void ucall_fmt(uint64_t cmd, const char *fmt, ...)
> > +{
> > + struct ucall *uc;
> > + va_list va;
> > +
> > + uc = ucall_alloc();
> > + uc->cmd = cmd;
> > +
> > + va_start(va, fmt);
> > + vsprintf(uc->buffer, fmt, va);
> > + va_end(va);
> > +
> > + ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> > +
> > + ucall_free(uc);
> > +}
> > +
> > void ucall(uint64_t cmd, int nargs, ...)
> > {
> > struct ucall *uc;
>
