Re: [Qemu-devel] [PATCH] target-i386: Enhance the stub for kvm_arch_get_supported_cpuid()

[Kamil Rytarowski]

Ping.

On 21.02.2019 03:08, Kamil Rytarowski wrote:
> On 20.02.2019 18:29, Paolo Bonzini wrote:
>> On 20/02/19 12:59, Kamil Rytarowski wrote:
>>> Ping, still valid.
>>
>> Sorry, I missed your email.
>>
>>> On 15.02.2019 00:38, Kamil Rytarowski wrote:
>>>> I consider it as fragile hack and certainly not something to depend on.
>>>> Also in some circumstances of such code, especially "if (zero0)" we want
>>>> to enable disabled code under a debugger.
>>
>> That's a good objection, but certainly does not apply to KVM on NetBSD.
>>
> 
> There is KVM for Darwin (experimental and rather toy project) and it
> might be ported to NetBSD (I have actually forked it on GitHub
> recently), but I doubt that someone would enable KVM on any platform
> under a debugger this way and expect something to work.
> 
>>>> There were also kernel backdoors due to this optimization.
>>
>> Citation please?
>>
> 
> I saw an exploit for such case with a .txt writeup on ftp of grsecurity
> but that service seems to be gone (probably long time ago), so please
> defer discussion on it. If someone is interested to find it out, there
> are enough pointers to dig it (assuming that this is still possible).
> 
>>>> Requested cpu.i (hopefully correctly generated)
>>>>
>>>> http://netbsd.org/~kamil/qemu/cpu.i.bz2
>>
>> So, first thing first I can reproduce clang's behavior with this .i file
>> and also with this reduced test case.
>>
>>     extern void f(void);
>>     int i, j;
>>     int main()
>>     {
>>         if (0  && i) f();
>>         if (j  && 0) f();
>>    }
>>
>> The first is eliminated but the second is not, just like in QEMU where
>> this works:
>>
>>         if (kvm_enabled() && cpu->enable_pmu) {
>>             KVMState *s = cs->kvm_state;
>>
>>             *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
>>             *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
>>             *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
>>             *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
>>         } else if (hvf_enabled() && cpu->enable_pmu) {
>>             *eax = hvf_get_supported_cpuid(0xA, count, R_EAX);
>>             *ebx = hvf_get_supported_cpuid(0xA, count, R_EBX);
>>             *ecx = hvf_get_supported_cpuid(0xA, count, R_ECX);
>>             *edx = hvf_get_supported_cpuid(0xA, count, R_EDX);
>>
>> while this doesn't:
>>
>>     if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
>>         kvm_enabled()) {
>>         KVMState *s = CPU(cpu)->kvm_state;
>>         uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
>>         uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
>>         uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
>>         uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
>>         uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
>>
>> But, that's okay, it's -O0 so we give clang a pass for that  Note that
>> clang does do the optimization even in more complex cases like
>>
>>     extern _Bool f(void);
>>     int main()
>>     {
>>         if (!0) return 0;
>>         if (!f()) return 0;
>>     }
>>
>> The problem is that there is a kvm-stub.c entry for that, and in fact
>> my compilation passes and the symbol is resolved correctly:
>>
>> $ nm target/i386/cpu.o |grep kvm_.*get_sup
>>                  U kvm_arch_get_supported_cpuid
>> $ nm target/i386/kvm-stub.o|grep kvm_.*get_sup
>> 0000000000000030 T kvm_arch_get_supported_cpuid
>> $ nm qemu-system-x86_64 |grep kvm_.*get_sup
>> 000000000046eab0 T kvm_arch_get_supported_cpuid
>>
>> As expected, something much less obvious is going on for you, in
>> particular __OPTIMIZE__seems not to be working properly.  However,
>> that would also be very surprising.
>>
>> Please:
>>
>> 1) run the last two "nm" commands on your build (wthout grep).
>>
> 
> I cannot run nm(1) on qemu-system-x86_64 as it's not linkable.
> 
> I'm getting the same result for target/i386/cpu.o and
> target/i386/kvm-stub.o.
> 
> $ nm ./i386-softmmu/target/i386/kvm-stub.o
>                  U abort
> 0000000000000000 T kvm_allows_irq0_override
> 0000000000000030 T kvm_arch_get_supported_cpuid
> 0000000000000020 T kvm_enable_x2apic
> 0000000000000010 T kvm_has_smm
> 0000000000000050 T kvm_hv_vpindex_settable
> 
> grep(1) used, but otherwise I would need to upload results somewhere else.
> 
> $ nm ./i386-bsd-user/target/i386/cpu.o |grep kvm
>                  U kvm_arch_get_supported_cpuid
> 0000000000001290 d kvm_default_props
>                  U kvm_state
> 0000000000000240 T x86_cpu_change_kvm_default
> 
> Please note that there are 4 types of x86 build: i386, x86_64 and two
> bsd-user (32-bit and 64-bit).
> 
> According to my observations of repeated attempts both builds of
> bsd-user are affected.
> 
> There is also a difference that kvm-stub is twice in !bsd-user
> directories and once in bsd-user ones.
> 
> $ find . -name kvm-stub.o|grep x86_64
> ./x86_64-softmmu/accel/stubs/kvm-stub.o
> ./x86_64-softmmu/target/i386/kvm-stub.o
> ./x86_64-bsd-user/accel/stubs/kvm-stub.o
> 
>> 2) do the same exercise to get a .i for target/i386/kvm-stub.c
>>
>> 3) try removing the "#ifndef __OPTIMIZE__" and leave everything else as is,
>> see if it works.
> 
> Same result as above, it seems that bsd-user ones are affected in the
> same way.
> 
>>  No need to play with macros, which also goes to show that
>> you didn't really understand what's going on---that's fine, but then please
>> refrain from making summary judgments which only lengthen the discussion.
>>
> 
> I stand with my expression that I find playing with optimizer as too
> fragile to depend on it. Relying on a preprocessor to disable unused
> code is predictable. But if that is the style in qemu, I can just
> acknowledge it.
> 
>> Thanks,
>>
>> Paolo
>>
> 
>

signature.asc
Description: OpenPGP digital signature