| Message ID | 20220525222604.2810054-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Emulator _regs fixes and cleanups | expand |
Sean Christopherson <seanjc@google.com> writes: > Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting > ctxt->regs_dirty to an 'unsigned long *' for use in for_each_set_bit(). > The bitops helpers really do read the entire 'unsigned long', even though > the walking of the read value is capped at the specified size. I.e. KVM > is reading memory beyond ctxt->regs_dirty. Functionally it's not an > issue because regs_dirty is in the middle of x86_emulate_ctxt, i.e. KVM > is just reading its own memory, but relying on that coincidence is gross > and unsafe. Your nearly perfect description misses one important part: in 'struct x86_emulate_ctxt', 'regs_dirty' is actually 'u32' thus all this buzz :-) > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/emulate.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 89b11e7dca8a..7226a127ccb4 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -269,9 +269,10 @@ static ulong *reg_rmw(struct x86_emulate_ctxt *ctxt, unsigned nr) > > static void writeback_registers(struct x86_emulate_ctxt *ctxt) > { > + unsigned long dirty = ctxt->regs_dirty; > unsigned reg; > > - for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16) > + for_each_set_bit(reg, &dirty, 16) > ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]); > } Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Wed, May 25, 2022 at 10:26:01PM +0000, Sean Christopherson wrote: > Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting > ctxt->regs_dirty to an 'unsigned long *' for use in for_each_set_bit(). > The bitops helpers really do read the entire 'unsigned long', even though > the walking of the read value is capped at the specified size. I.e. KVM > is reading memory beyond ctxt->regs_dirty. Functionally it's not an > issue because regs_dirty is in the middle of x86_emulate_ctxt, i.e. KVM > is just reading its own memory, but relying on that coincidence is gross > and unsafe. Right; there have bean a handful of other bitops-using-stuff-cast-to-ulong that has been recently fixed elsewhere. Better to get them all squashed. :) > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 89b11e7dca8a..7226a127ccb4 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -269,9 +269,10 @@ static ulong *reg_rmw(struct x86_emulate_ctxt *ctxt, unsigned nr) static void writeback_registers(struct x86_emulate_ctxt *ctxt) { + unsigned long dirty = ctxt->regs_dirty; unsigned reg; - for_each_set_bit(reg, (ulong *)&ctxt->regs_dirty, 16) + for_each_set_bit(reg, &dirty, 16) ctxt->ops->write_gpr(ctxt, reg, ctxt->_regs[reg]); }
Capture ctxt->regs_dirty in a local 'unsigned long' instead of casting ctxt->regs_dirty to an 'unsigned long *' for use in for_each_set_bit(). The bitops helpers really do read the entire 'unsigned long', even though the walking of the read value is capped at the specified size. I.e. KVM is reading memory beyond ctxt->regs_dirty. Functionally it's not an issue because regs_dirty is in the middle of x86_emulate_ctxt, i.e. KVM is just reading its own memory, but relying on that coincidence is gross and unsafe. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/emulate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)