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

[Kamil Rytarowski]

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