linux-kernel - Re: [PATCH 3/3] KVM: x86: move kvm_inject_gp up from kvm_set

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] [day] [month] [year] [list]

Message-ID: <63f19b57-7189-4b36-5159-a42df15336a5@redhat.com>
Date:   Wed, 3 Feb 2021 10:24:00 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 3/3] KVM: x86: move kvm_inject_gp up from kvm_set_dr to
 callers

On 02/02/21 19:17, Sean Christopherson wrote:
>> @@ -2617,19 +2618,18 @@ static int dr_interception(struct vcpu_svm *svm)
>>   	reg = svm->vmcb->control.exit_info_1 & SVM_EXITINFO_REG_MASK;
>>   	dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
>>   
>> +	if (!kvm_require_dr(&svm->vcpu, dr & 15))
> 
> Purely because I suck at reading base-10 bitwise operations, can we do "dr & 0xf"?

I would have never said that having this => 
https://www.youtube.com/watch?v=nfUY3_XVKHI as a kid would give me a 
competitive advantage as KVM maintainer. :)

(Aside: that game was incredibly popular in the 80s in Italy and as you 
can see from the advertisement at 
https://www.youtube.com/watch?v=o6L9cegnCrw it even had "the binary 
teacher" in it, yes in English despite no one spoke English fluently at 
the time.  The guy who invented it was an absolute genius.  Also, the 
name means "branches").

But seriously: I think the usage of "-" was intentional because the AMD 
exit codes have READ first and WRITE second---but it's (almost) a 
coincidence that CR/DR intercepts are naturally aligned and bit 4 means 
read vs. write.

So v2 will remove the kvm_require_dr (I tested your hypothesis with 
debug.flat and KVM_DEBUGREG_WONT_EXIT disabled, and you're right) and have:

         dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0;
         if (dr >= 16) { /* Move to dr.  */
                 dr -= 16;
                 val = kvm_register_read(&svm->vcpu, reg);
                 err = kvm_set_dr(&svm->vcpu, dr, val);
         } else {
                 kvm_get_dr(&svm->vcpu, dr, &val);
                 kvm_register_write(&svm->vcpu, reg, val);
         }

Paolo

> Technically, 'err' needs to be checked, else 'reg' will theoretically be
> clobbered with garbage.  I say "theoretically", because kvm_get_dr() always
> returns '0'; the CR4.DE=1 behavior is handled by kvm_require_dr(), presumably
> due to it being a #UD instead of #GP.  AFAICT, you can simply add a prep patch
> to change the return type to void.
> 
> Side topic, is the kvm_require_dr() check needed on SVM interception?  The APM
> states:
> 
>    All normal exception checks take precedence over the by implicit DR6/DR7 writes.)
> 
> I can't find anything that would suggest the CR4.DE=1 #UD isn't a "normal"
> exception.
> 
>>   		kvm_register_write(&svm->vcpu, reg, val);
>>   	}
>>   
>> -	return kvm_skip_emulated_instruction(&svm->vcpu);
>> +	return kvm_complete_insn_gp(&svm->vcpu, err);
>>   }
>>   
>>   static int cr8_write_interception(struct vcpu_svm *svm)
>