| 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
| ||
|
Message-ID: <202108180922.6C9E385A1@keescook>
Date: Wed, 18 Aug 2021 09:23:43 -0700
From: Kees Cook <keescook@...omium.org>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
kvm@...r.kernel.org, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-staging@...ts.linux.dev,
linux-block@...r.kernel.org, linux-kbuild@...r.kernel.org,
clang-built-linux@...glegroups.com,
Rasmus Villemoes <linux@...musvillemoes.dk>,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 53/63] KVM: x86: Use struct_group() to zero decode
cache
On Wed, Aug 18, 2021 at 03:11:28PM +0000, Sean Christopherson wrote:
> On Tue, Aug 17, 2021, Kees Cook wrote:
> > arch/x86/kvm/emulate.c | 3 +--
> > arch/x86/kvm/kvm_emulate.h | 19 +++++++++++--------
> > 2 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 2837110e66ed..2608a047e769 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -5377,8 +5377,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop)
> >
> > void init_decode_cache(struct x86_emulate_ctxt *ctxt)
> > {
> > - memset(&ctxt->rip_relative, 0,
> > - (void *)&ctxt->modrm - (void *)&ctxt->rip_relative);
> > + memset(&ctxt->decode_cache, 0, sizeof(ctxt->decode_cache));
> >
> > ctxt->io_read.pos = 0;
> > ctxt->io_read.end = 0;
> > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> > index 68b420289d7e..9b8afcb8ad39 100644
> > --- a/arch/x86/kvm/kvm_emulate.h
> > +++ b/arch/x86/kvm/kvm_emulate.h
> > @@ -341,14 +341,17 @@ struct x86_emulate_ctxt {
> > * the rest are initialized unconditionally in x86_decode_insn
> > * or elsewhere
> > */
> > - bool rip_relative;
> > - u8 rex_prefix;
> > - u8 lock_prefix;
> > - u8 rep_prefix;
> > - /* bitmaps of registers in _regs[] that can be read */
> > - u32 regs_valid;
> > - /* bitmaps of registers in _regs[] that have been written */
> > - u32 regs_dirty;
> > + struct_group(decode_cache,
>
> This is somewhat misleading because half of this struct is the so called "decode
> cache", not just these six fields.
>
> KVM's "optimization" is quite ridiculous as this has never been such a hot path
> that saving a few mov instructions would be noticeable. And hilariously, the
> "optimization" is completely unnecessary because both gcc and clang are clever
> enough to batch the first five into a movq even when zeroing the fields individually.
>
> So, I would much prefer to go with the following:
Sounds good to me!
>
> From dbdca1f4cd01fee418c252e54c360d518b2b1ad6 Mon Sep 17 00:00:00 2001
> From: Sean Christopherson <seanjc@...gle.com>
> Date: Wed, 18 Aug 2021 08:03:08 -0700
> Subject: [PATCH] KVM: x86: Replace memset() "optimization" with normal
> per-field writes
>
> Explicitly zero select fields in the emulator's decode cache instead of
> zeroing the fields via a gross memset() that spans six fields. gcc and
> clang are both clever enough to batch the first five fields into a single
> quadword MOV, i.e. memset() and individually zeroing generate identical
> code.
>
> Removing the wart also prepares KVM for FORTIFY_SOURCE performing
> compile-time and run-time field bounds checking for memset().
>
> No functional change intended.
>
> Reported-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
Reviewed-by: Kees Cook <keescook@...omium.org>
Do you want me to take this patch into my tree, or do you want to carry
it for KVM directly?
Thanks!
-Kees
> ---
> arch/x86/kvm/emulate.c | 9 +++++++--
> arch/x86/kvm/kvm_emulate.h | 6 +-----
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 2837110e66ed..bf81fd017e7f 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5377,8 +5377,13 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop)
>
> void init_decode_cache(struct x86_emulate_ctxt *ctxt)
> {
> - memset(&ctxt->rip_relative, 0,
> - (void *)&ctxt->modrm - (void *)&ctxt->rip_relative);
> + /* Clear fields that are set conditionally but read without a guard. */
> + ctxt->rip_relative = false;
> + ctxt->rex_prefix = 0;
> + ctxt->lock_prefix = 0;
> + ctxt->rep_prefix = 0;
> + ctxt->regs_valid = 0;
> + ctxt->regs_dirty = 0;
>
> ctxt->io_read.pos = 0;
> ctxt->io_read.end = 0;
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 68b420289d7e..bc1fecacccd4 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -336,11 +336,7 @@ struct x86_emulate_ctxt {
> fastop_t fop;
> };
> int (*check_perm)(struct x86_emulate_ctxt *ctxt);
> - /*
> - * The following six fields are cleared together,
> - * the rest are initialized unconditionally in x86_decode_insn
> - * or elsewhere
> - */
> +
> bool rip_relative;
> u8 rex_prefix;
> u8 lock_prefix;
> --
> 2.33.0.rc1.237.g0d66db33f3-goog
>
> > + bool rip_relative;
> > + u8 rex_prefix;
> > + u8 lock_prefix;
> > + u8 rep_prefix;
> > + /* bitmaps of registers in _regs[] that can be read */
> > + u32 regs_valid;
> > + /* bitmaps of registers in _regs[] that have been written */
> > + u32 regs_dirty;
> > + );
> > +
> > /* modrm */
> > u8 modrm;
> > u8 modrm_mod;
> > --
> > 2.30.2
> >
--
Kees Cook
Powered by blists - more mailing lists