linux-kernel - Re: [PATCH v10 10/28] x86/fpu/xstate: Update the XSTATE save function to support dynamic states

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87pmsnglkr.ffs@tglx>
Date:   Sat, 02 Oct 2021 23:31:48 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>, bp@...e.de,
        luto@...nel.org, mingo@...nel.org, x86@...nel.org
Cc:     len.brown@...el.com, lenb@...nel.org, dave.hansen@...el.com,
        thiago.macieira@...el.com, jing2.liu@...el.com,
        ravi.v.shankar@...el.com, linux-kernel@...r.kernel.org,
        chang.seok.bae@...el.com, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v10 10/28] x86/fpu/xstate: Update the XSTATE save
 function to support dynamic states

On Fri, Oct 01 2021 at 17:41, Thomas Gleixner wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 74dde635df40..7c46747f6865 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -9899,11 +9899,16 @@ static void kvm_save_current_fpu(struct fpu *fpu)
>>  	 * KVM does not support dynamic user states yet. Assume the buffer
>>  	 * always has the minimum size.

I have to come back to this because that assumption is just broken.

create_vcpu()
   vcpu->user_fpu = alloc_default_fpu_size();
   vcpu->guest_fpu = alloc_default_fpu_size();

vcpu_task()
   get_amx_permission()
   use_amx()
     #NM
     alloc_larger_state()
   ...
   kvm_arch_vcpu_ioctl_run()
     kvm_arch_vcpu_ioctl_run()
       kvm_load_guest_fpu()
         kvm_save_current_fpu(vcpu->arch.user_fpu);
           save_fpregs_to_fpstate(fpu);         <- Out of bounds write

Adding a comment that KVM does not yet support dynamic user states does
not cut it, really.

Even if the above is unlikely, it is possible and has to be handled
correctly at the point where AMX support is enabled in the kernel
independent of guest support.

You have two options:

  1) Always allocate the large buffer size which is required to
     accomodate all possible features.

     Trivial, but waste of memory.

  2) Make the allocation dynamic which seems to be trivial to do in
     kvm_load_guest_fpu() at least for vcpu->user_fpu.

     The vcpu->guest_fpu handling can probably be postponed to the
     point where AMX is actually exposed to guests, but it's probably
     not the worst idea to think about the implications now.

Paolo, any opinions?

Thanks,

        tglx