That seems like a convoluted path to produce an illegal RFLAGS value.
What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
the KVM_SET_REGS ioctl?
On Mon, Nov 20, 2017 at 4:34 PM, Wanpeng Li <[email protected]> wrote:
> 2017-11-21 7:09 GMT+08:00 Paolo Bonzini <[email protected]>:
>> On 20/11/2017 23:52, Wanpeng Li wrote:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index b348920..131fa1c 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>> vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>>> }
>>>
>>> + kvm_set_rflags(vcpu, 2);
>>> vmcs_writel(GUEST_RFLAGS, 0x02);
>>
>> I think the vmcs_writel can go, kvm_set_rflags ends up calling
>> vmx_set_rflags.
>
> Agreed.
>
>>
>>> kvm_rip_write(vcpu, 0xfff0);
>>>
>>>
>>
>> Beautified testcase:
>>
>> #include <unistd.h>
>> #include <sys/syscall.h>
>> #include <string.h>
>> #include <stdint.h>
>> #include <linux/kvm.h>
>> #include <fcntl.h>
>> #include <sys/ioctl.h>
>>
>> long r[5];
>> int main()
>> {
>> struct kvm_debugregs dr = { 0 };
>>
>> r[2] = open("/dev/kvm", O_RDONLY);
>> r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>> r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
>> struct kvm_guest_debug debug = {
>> .control = 0xf0403,
>> .arch = {
>> .debugreg[6] = 0x2,
>> .debugreg[7] = 0x2
>> }
>> };
>> ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
>> ioctl(r[4], KVM_RUN, 0);
>> }
>>
>> No need to do anything, I'll handle this patch after -rc1.
>
> Thanks for that. :)
>
> Regards,
> Wanpeng Li
2017-12-05 8:28 GMT+08:00 Jim Mattson <[email protected]>:
> That seems like a convoluted path to produce an illegal RFLAGS value.
> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
> the KVM_SET_REGS ioctl?
Yeah, it can happen. Which do you prefer, ioctl fails or |
X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?
Regards,
Wanpeng Li
>
> On Mon, Nov 20, 2017 at 4:34 PM, Wanpeng Li <[email protected]> wrote:
>> 2017-11-21 7:09 GMT+08:00 Paolo Bonzini <[email protected]>:
>>> On 20/11/2017 23:52, Wanpeng Li wrote:
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index b348920..131fa1c 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -5590,6 +5590,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>> vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
>>>> }
>>>>
>>>> + kvm_set_rflags(vcpu, 2);
>>>> vmcs_writel(GUEST_RFLAGS, 0x02);
>>>
>>> I think the vmcs_writel can go, kvm_set_rflags ends up calling
>>> vmx_set_rflags.
>>
>> Agreed.
>>
>>>
>>>> kvm_rip_write(vcpu, 0xfff0);
>>>>
>>>>
>>>
>>> Beautified testcase:
>>>
>>> #include <unistd.h>
>>> #include <sys/syscall.h>
>>> #include <string.h>
>>> #include <stdint.h>
>>> #include <linux/kvm.h>
>>> #include <fcntl.h>
>>> #include <sys/ioctl.h>
>>>
>>> long r[5];
>>> int main()
>>> {
>>> struct kvm_debugregs dr = { 0 };
>>>
>>> r[2] = open("/dev/kvm", O_RDONLY);
>>> r[3] = ioctl(r[2], KVM_CREATE_VM, 0);
>>> r[4] = ioctl(r[3], KVM_CREATE_VCPU, 7);
>>> struct kvm_guest_debug debug = {
>>> .control = 0xf0403,
>>> .arch = {
>>> .debugreg[6] = 0x2,
>>> .debugreg[7] = 0x2
>>> }
>>> };
>>> ioctl(r[4], KVM_SET_GUEST_DEBUG, &debug);
>>> ioctl(r[4], KVM_RUN, 0);
>>> }
>>>
>>> No need to do anything, I'll handle this patch after -rc1.
>>
>> Thanks for that. :)
>>
>> Regards,
>> Wanpeng Li
On 05/12/2017 01:53, Wanpeng Li wrote:
>> That seems like a convoluted path to produce an illegal RFLAGS value.
>> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
>> the KVM_SET_REGS ioctl?
> Yeah, it can happen. Which do you prefer, ioctl fails or |
> X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?
I suspect somebody might be passing an all-zero regs struct to
KVM_SET_REGS, so ORing X86_EFLAGS_FIXED is better.
Thanks,
Paolo
Sorry; I didn't mean to derail this patch thread. Setting bit 1 of
RFLAGS on CPU reset is clearly correct.
I was just noting that if syzkaller is complaining about illegal
RFLAGS, it's trivial for userspace to set RFLAGS to an illegal value.
User space can set all kinds of illegal RFLAGS state...bits 63:22, 15,
5, or 3 can be set; bit 1 can be cleared; RFLAGS.VM can be set in long
mode or real-address mode; RFLAGS.IF can be set while injecting an
interrupt via KVM_SET_VCPU_EVENTS. Let's not get hung up too much on
the one case of bit 1 being clear.
I don't think KVM_SET_REGS, KVM_SET_SREGS, KVM_SET_VCPU_EVENTS, and
friends should necessarily be in the validation business. There are
too many context-dependent validation checks that might pass or fail
depending on the order in which these ioctls are called. In a way,
these ioctls are comparable to the VMWRITE instruction, which does no
validity checking.
Better, perhaps, would be to defer the validity checks to KVM_VMRUN
(and let the hardware take care of them). Unfortunately, this doesn't
interact very well with emulate_invalid_guest_state = true and
enable_unrestricted_guest = false, in which case invalid guest state
is passed to the kernel emulator to see how well it deals with the
illegal state. (By the way, that's a really fun configuration for
syzkaller, because it opens the door to a myriad of kvm warnings that
are difficult to exercise otherwise).
So, how should illegal RFLAGS bits be handled by KVM_SET_REGS? Sure,
we could set bit 1, and we could clear bits 63:22, 15, 5, and 3. But
what about RFLAGS.VM and RFLAGS.IF?
On Tue, Dec 5, 2017 at 5:54 AM, Paolo Bonzini <[email protected]> wrote:
> On 05/12/2017 01:53, Wanpeng Li wrote:
>>> That seems like a convoluted path to produce an illegal RFLAGS value.
>>> What's to prevent syzkaller from simply clearing bit 1 of RFLAGS with
>>> the KVM_SET_REGS ioctl?
>> Yeah, it can happen. Which do you prefer, ioctl fails or |
>> X86_EFLAGS_FIXED unconditionally in the ioctl handler in kvm?
>
> I suspect somebody might be passing an all-zero regs struct to
> KVM_SET_REGS, so ORing X86_EFLAGS_FIXED is better.
>
> Thanks,
>
> Paolo