On 04.08.25 05:32, Xiaoyao Li wrote:
> On 8/1/2025 9:12 PM, Mathias Krause wrote:
>> [...]
>>
>> For regular operating systems, however, the patching wouldn't be needed,
>> nor work at all. If it would, these systrems would be vulnerable to
>> memory corruption attacks, freely overwriting kernel code as they
>> please.
>
> For non-coco VMs, the systems are surely vulnerable to memory corruption
> attacks that the host VMM is free to modify the guest memory. It's
> irrelevant to whether hypercall patching is needed or works.
Sure, a VMM could mess with the guest's memory as it pleases. However, I
meant possible attacks from *within* the guest, as in allowing code
modifications to happen by having W+X mappings, allowing possibly
malicious modifications of such.
>> [...]
>> ---
>> Xiaoyao, I left out your Tested-by and Reviewed-by as I changed the code
>> (slightly) and it didn't felt right to pick these up. However, as only
>> the default value changed, the functionality would be the same if you
>> tested both cases explicitly (-accel kvm,hypercall-patching={on,off}).
>
> No problem, I just re-tested it.
>
> Tested-by: Xiaoyao Li <xiaoyao...@intel.com>
> Reviewed-by: Xiaoyao Li <xiaoyao...@intel.com>
Thanks!
>> [...]
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index ab23f14d2178..98af1a91e6e6 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -236,6 +236,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
>> " dirty-ring-size=n (KVM dirty ring GFN count,
>> default 0)\n"
>> " eager-split-size=n (KVM Eager Page Split chunk
>> size, default 0, disabled. ARM only)\n"
>> " notify-vmexit=run|internal-error|
>> disable,notify-window=n (enable notify VM exit and set notify window,
>> x86 only)\n"
>> + " hypercall-patching=on|off (disable KVM's VMCALL/
>> VMMCALL hypercall patching quirk, x86 only)\n"
>
> I would like to say "(configure KVM's VMCALL/VMCALL hypercall patching
> quirk, x86 only)" instead of "disable"
That would be technically correct. However, as this quirk is enabled by
default in KVM and QEMU, the only sensible configuration toggle is to
disable it. That's why I stated it this way. But I can rephrase it, if
you prefer it this way.
>> [...]
>> @@ -6611,6 +6650,12 @@ static void
>> kvm_arch_set_xen_evtchn_max_pirq(Object *obj, Visitor *v,
>> void kvm_arch_accel_class_init(ObjectClass *oc)
>> {
>> + object_class_property_add_bool(oc, "hypercall-patching",
>> + kvm_arch_get_hypercall_patching,
>> + kvm_arch_set_hypercall_patching);
>> + object_class_property_set_description(oc, "hypercall-patching",
>> + "Disable hypercall patching
>> quirk");
>
> Ditto, Could we use "Configure hypercall patching quirk"? It's not only
> to disable it.
Or just "Hypercall patching quirk", as the bool value already reflects
its state.
Thanks,
Mathias
