* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-06 3:03 [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit Andy Lutomirski
@ 2014-12-06 15:30 ` Andy Lutomirski
2014-12-08 15:45 ` Konrad Rzeszutek Wilk
2014-12-10 11:49 ` Paolo Bonzini
2 siblings, 0 replies; 7+ messages in thread
From: Andy Lutomirski @ 2014-12-06 15:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm list, Rusty Russell, Konrad Rzeszutek Wilk, Andy Lutomirski,
stable
On Fri, Dec 5, 2014 at 7:03 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> paravirt_enabled has the following effects:
>
> - Disables the F00F bug workaround warning. There is no F00F bug
> workaround any more because Linux's standard IDT handling already
> works around the F00F bug, but the warning still exists. This
> is only cosmetic, and, in any event, there is no such thing as
> KVM on a CPU with the F00F bug.
>
> - Disables 32-bit APM BIOS detection. On a KVM paravirt system,
> there should be no APM BIOS anyway.
>
> - Disables tboot. I think that the tboot code should check the
> CPUID hypervisor bit directly if it matters.
>
> - paravirt_enabled disables espfix32. espfix32 should *not* be
> disabled under KVM paravirt.
>
> The last point is the purpose of this patch. It fixes a leak of the
> high 16 bits of the kernel stack address on 32-bit KVM paravirt
> guests.
>
> While I'm at it, this removes pv_info setup from kvmclock. That
> code seems to serve no purpose.
If you apply this, please add:
Fixes CVE-2014-8134.
Thanks,
Andy
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
> arch/x86/kernel/kvm.c | 9 ++++++++-
> arch/x86/kernel/kvmclock.c | 2 --
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index f6945bef2cd1..94f643484300 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -283,7 +283,14 @@ NOKPROBE_SYMBOL(do_async_page_fault);
> static void __init paravirt_ops_setup(void)
> {
> pv_info.name = "KVM";
> - pv_info.paravirt_enabled = 1;
> +
> + /*
> + * KVM isn't paravirt in the sense of paravirt_enabled. A KVM
> + * guest kernel works like a bare metal kernel with additional
> + * features, and paravirt_enabled is about features that are
> + * missing.
> + */
> + pv_info.paravirt_enabled = 0;
>
> if (kvm_para_has_feature(KVM_FEATURE_NOP_IO_DELAY))
> pv_cpu_ops.io_delay = kvm_io_delay;
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index d9156ceecdff..d4d9a8ad7893 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -263,8 +263,6 @@ void __init kvmclock_init(void)
> #endif
> kvm_get_preset_lpj();
> clocksource_register_hz(&kvm_clock, NSEC_PER_SEC);
> - pv_info.paravirt_enabled = 1;
> - pv_info.name = "KVM";
>
> if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
> pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
> --
> 1.9.3
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-06 3:03 [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit Andy Lutomirski
2014-12-06 15:30 ` Andy Lutomirski
@ 2014-12-08 15:45 ` Konrad Rzeszutek Wilk
2014-12-08 16:43 ` Andy Lutomirski
2014-12-10 11:49 ` Paolo Bonzini
2 siblings, 1 reply; 7+ messages in thread
From: Konrad Rzeszutek Wilk @ 2014-12-08 15:45 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Paolo Bonzini, kvm list, Rusty Russell, stable
On Fri, Dec 05, 2014 at 07:03:28PM -0800, Andy Lutomirski wrote:
> paravirt_enabled has the following effects:
>
> - Disables the F00F bug workaround warning. There is no F00F bug
> workaround any more because Linux's standard IDT handling already
> works around the F00F bug, but the warning still exists. This
> is only cosmetic, and, in any event, there is no such thing as
> KVM on a CPU with the F00F bug.
>
> - Disables 32-bit APM BIOS detection. On a KVM paravirt system,
> there should be no APM BIOS anyway.
>
> - Disables tboot. I think that the tboot code should check the
> CPUID hypervisor bit directly if it matters.
>
> - paravirt_enabled disables espfix32. espfix32 should *not* be
> disabled under KVM paravirt.
>
> The last point is the purpose of this patch. It fixes a leak of the
> high 16 bits of the kernel stack address on 32-bit KVM paravirt
> guests.
>
> While I'm at it, this removes pv_info setup from kvmclock. That
> code seems to serve no purpose.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Suggested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> arch/x86/kernel/kvm.c | 9 ++++++++-
> arch/x86/kernel/kvmclock.c | 2 --
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index f6945bef2cd1..94f643484300 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -283,7 +283,14 @@ NOKPROBE_SYMBOL(do_async_page_fault);
> static void __init paravirt_ops_setup(void)
> {
> pv_info.name = "KVM";
> - pv_info.paravirt_enabled = 1;
> +
> + /*
> + * KVM isn't paravirt in the sense of paravirt_enabled. A KVM
> + * guest kernel works like a bare metal kernel with additional
> + * features, and paravirt_enabled is about features that are
> + * missing.
> + */
> + pv_info.paravirt_enabled = 0;
>
> if (kvm_para_has_feature(KVM_FEATURE_NOP_IO_DELAY))
> pv_cpu_ops.io_delay = kvm_io_delay;
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index d9156ceecdff..d4d9a8ad7893 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -263,8 +263,6 @@ void __init kvmclock_init(void)
> #endif
> kvm_get_preset_lpj();
> clocksource_register_hz(&kvm_clock, NSEC_PER_SEC);
> - pv_info.paravirt_enabled = 1;
> - pv_info.name = "KVM";
>
> if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
> pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-08 15:45 ` Konrad Rzeszutek Wilk
@ 2014-12-08 16:43 ` Andy Lutomirski
0 siblings, 0 replies; 7+ messages in thread
From: Andy Lutomirski @ 2014-12-08 16:43 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Paolo Bonzini, kvm list, Rusty Russell, stable
On Mon, Dec 8, 2014 at 7:45 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Dec 05, 2014 at 07:03:28PM -0800, Andy Lutomirski wrote:
>> paravirt_enabled has the following effects:
>>
>> - Disables the F00F bug workaround warning. There is no F00F bug
>> workaround any more because Linux's standard IDT handling already
>> works around the F00F bug, but the warning still exists. This
>> is only cosmetic, and, in any event, there is no such thing as
>> KVM on a CPU with the F00F bug.
>>
>> - Disables 32-bit APM BIOS detection. On a KVM paravirt system,
>> there should be no APM BIOS anyway.
>>
>> - Disables tboot. I think that the tboot code should check the
>> CPUID hypervisor bit directly if it matters.
>>
>> - paravirt_enabled disables espfix32. espfix32 should *not* be
>> disabled under KVM paravirt.
>>
>> The last point is the purpose of this patch. It fixes a leak of the
>> high 16 bits of the kernel stack address on 32-bit KVM paravirt
>> guests.
>>
>> While I'm at it, this removes pv_info setup from kvmclock. That
>> code seems to serve no purpose.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>
> Suggested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Sorry, meant to add that but forgot. Too many patches late last week :(
>> ---
>> arch/x86/kernel/kvm.c | 9 ++++++++-
>> arch/x86/kernel/kvmclock.c | 2 --
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index f6945bef2cd1..94f643484300 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -283,7 +283,14 @@ NOKPROBE_SYMBOL(do_async_page_fault);
>> static void __init paravirt_ops_setup(void)
>> {
>> pv_info.name = "KVM";
>> - pv_info.paravirt_enabled = 1;
>> +
>> + /*
>> + * KVM isn't paravirt in the sense of paravirt_enabled. A KVM
>> + * guest kernel works like a bare metal kernel with additional
>> + * features, and paravirt_enabled is about features that are
>> + * missing.
>> + */
>> + pv_info.paravirt_enabled = 0;
>>
>> if (kvm_para_has_feature(KVM_FEATURE_NOP_IO_DELAY))
>> pv_cpu_ops.io_delay = kvm_io_delay;
>> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
>> index d9156ceecdff..d4d9a8ad7893 100644
>> --- a/arch/x86/kernel/kvmclock.c
>> +++ b/arch/x86/kernel/kvmclock.c
>> @@ -263,8 +263,6 @@ void __init kvmclock_init(void)
>> #endif
>> kvm_get_preset_lpj();
>> clocksource_register_hz(&kvm_clock, NSEC_PER_SEC);
>> - pv_info.paravirt_enabled = 1;
>> - pv_info.name = "KVM";
>>
>> if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
>> pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>> --
>> 1.9.3
>>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-06 3:03 [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit Andy Lutomirski
2014-12-06 15:30 ` Andy Lutomirski
2014-12-08 15:45 ` Konrad Rzeszutek Wilk
@ 2014-12-10 11:49 ` Paolo Bonzini
2014-12-10 20:46 ` Andy Lutomirski
2 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2014-12-10 11:49 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: kvm list, Rusty Russell, Konrad Rzeszutek Wilk, stable
On 06/12/2014 04:03, Andy Lutomirski wrote:
> paravirt_enabled has the following effects:
>
> - Disables the F00F bug workaround warning. There is no F00F bug
> workaround any more because Linux's standard IDT handling already
> works around the F00F bug, but the warning still exists. This
> is only cosmetic, and, in any event, there is no such thing as
> KVM on a CPU with the F00F bug.
>
> - Disables 32-bit APM BIOS detection. On a KVM paravirt system,
> there should be no APM BIOS anyway.
>
> - Disables tboot. I think that the tboot code should check the
> CPUID hypervisor bit directly if it matters.
>
> - paravirt_enabled disables espfix32. espfix32 should *not* be
> disabled under KVM paravirt.
>
> The last point is the purpose of this patch. It fixes a leak of the
> high 16 bits of the kernel stack address on 32-bit KVM paravirt
> guests.
>
> While I'm at it, this removes pv_info setup from kvmclock. That
> code seems to serve no purpose.
kvmclock_init runs before kvm_guest_init, and this is a stable@ patch so
for the sake of extra safety I've left the pv_info.name assignment in.
Applied (locally for now), will be in 3.19.
Paolo
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
> arch/x86/kernel/kvm.c | 9 ++++++++-
> arch/x86/kernel/kvmclock.c | 2 --
> 2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index f6945bef2cd1..94f643484300 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -283,7 +283,14 @@ NOKPROBE_SYMBOL(do_async_page_fault);
> static void __init paravirt_ops_setup(void)
> {
> pv_info.name = "KVM";
> - pv_info.paravirt_enabled = 1;
> +
> + /*
> + * KVM isn't paravirt in the sense of paravirt_enabled. A KVM
> + * guest kernel works like a bare metal kernel with additional
> + * features, and paravirt_enabled is about features that are
> + * missing.
> + */
> + pv_info.paravirt_enabled = 0;
>
> if (kvm_para_has_feature(KVM_FEATURE_NOP_IO_DELAY))
> pv_cpu_ops.io_delay = kvm_io_delay;
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index d9156ceecdff..d4d9a8ad7893 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -263,8 +263,6 @@ void __init kvmclock_init(void)
> #endif
> kvm_get_preset_lpj();
> clocksource_register_hz(&kvm_clock, NSEC_PER_SEC);
> - pv_info.paravirt_enabled = 1;
> - pv_info.name = "KVM";
>
> if (kvm_para_has_feature(KVM_FEATURE_CLOCKSOURCE_STABLE_BIT))
> pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT);
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-10 11:49 ` Paolo Bonzini
@ 2014-12-10 20:46 ` Andy Lutomirski
2014-12-10 21:05 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Andy Lutomirski @ 2014-12-10 20:46 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm list, Rusty Russell, Konrad Rzeszutek Wilk, stable
On Wed, Dec 10, 2014 at 3:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 06/12/2014 04:03, Andy Lutomirski wrote:
>> paravirt_enabled has the following effects:
>>
>> - Disables the F00F bug workaround warning. There is no F00F bug
>> workaround any more because Linux's standard IDT handling already
>> works around the F00F bug, but the warning still exists. This
>> is only cosmetic, and, in any event, there is no such thing as
>> KVM on a CPU with the F00F bug.
>>
>> - Disables 32-bit APM BIOS detection. On a KVM paravirt system,
>> there should be no APM BIOS anyway.
>>
>> - Disables tboot. I think that the tboot code should check the
>> CPUID hypervisor bit directly if it matters.
>>
>> - paravirt_enabled disables espfix32. espfix32 should *not* be
>> disabled under KVM paravirt.
>>
>> The last point is the purpose of this patch. It fixes a leak of the
>> high 16 bits of the kernel stack address on 32-bit KVM paravirt
>> guests.
>>
>> While I'm at it, this removes pv_info setup from kvmclock. That
>> code seems to serve no purpose.
>
> kvmclock_init runs before kvm_guest_init, and this is a stable@ patch so
> for the sake of extra safety I've left the pv_info.name assignment in.
> Applied (locally for now), will be in 3.19.
>
In the interest of reduced future confusion, would it make sense to
drop the duplicate initialization for 3.20?
--Andy
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86, kvm: Clear paravirt_enabled on KVM guests for espfix32's benefit
2014-12-10 20:46 ` Andy Lutomirski
@ 2014-12-10 21:05 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2014-12-10 21:05 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: kvm list, Rusty Russell, Konrad Rzeszutek Wilk, stable
> In the interest of reduced future confusion, would it make sense to
> drop the duplicate initialization for 3.20?
Yup. It would be great if possible to even unify the two init
functions, but I haven't checked what happens in the middle.
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread