Re: [PATCH] target/i386: kvm: do not access uninitialized variable on older kernels

[Volker Rümelin]

Am 18.03.22 um 16:26 schrieb Paolo Bonzini:
> KVM support for AMX includes a new system attribute, KVM_X86_XCOMP_GUEST_SUPP.
> Commit 19db68ca68 ("x86: Grant AMX permission for guest", 2022-03-15) however
> did not fully consider the behavior on older kernels.  First, it warns
> too aggressively.  Second, it invokes the KVM_GET_DEVICE_ATTR ioctl
> unconditionally and then uses the "bitmask" variable, which remains
> uninitialized if the ioctl fails.
>
> While at it, explain why the ioctl is needed and KVM_GET_SUPPORTED_CPUID
> is not enough.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/kvm/kvm.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index ce0e8a4042..f2c9f7b5ca 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -412,6 +412,12 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, 
> uint32_t function,
>           }
>       } else if (function == 0xd && index == 0 &&
>                  (reg == R_EAX || reg == R_EDX)) {
> +        /*
> +         * The value returned by KVM_GET_SUPPORTED_CPUID does not include
> +         * features that still have to be enabled with the arch_prctl
> +         * system call.  QEMU needs the full value, which is retrieved
> +         * with KVM_GET_DEVICE_ATTR.
> +         */
>           struct kvm_device_attr attr = {
>               .group = 0,
>               .attr = KVM_X86_XCOMP_GUEST_SUPP,
> @@ -420,13 +426,16 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, 
> uint32_t function,
>   
>           bool sys_attr = kvm_check_extension(s, KVM_CAP_SYS_ATTRIBUTES);
>           if (!sys_attr) {
> -            warn_report("cannot get sys attribute capabilities %d", sys_attr);
> +            return ret;
>           }
>   
>           int rc = kvm_ioctl(s, KVM_GET_DEVICE_ATTR, &attr);
> -        if (rc == -1 && (errno == ENXIO || errno == EINVAL)) {
> -            warn_report("KVM_GET_DEVICE_ATTR(0, KVM_X86_XCOMP_GUEST_SUPP) "
> -                        "error: %d", rc);
> +        if (rc == -1) {

Hi Paolo,

this is kvm_ioctl() not ioctl(). kvm_ioctl() returns -errno on error.

With best regards,
Volker

> +            if (errno != ENXIO) {
> +                warn_report("KVM_GET_DEVICE_ATTR(0, KVM_X86_XCOMP_GUEST_SUPP) "
> +                            "error: %d", rc);
> +            }
> +            return ret;
>           }
>           ret = (reg == R_EAX) ? bitmask : bitmask >> 32;
>       } else if (function == 0x80000001 && reg == R_ECX) {