linux-kernel - Re: [PATCH] KVM: x86: dynamically allocate large struct in em

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 May 2017 18:36:54 -0700
From:   Nick Desaulniers <nick.desaulniers@...il.com>
To:     Radim Krčmář <rkrcmar@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: dynamically allocate large struct in em_fxrstor

On Wed, May 24, 2017 at 04:19:57PM +0200, Radim Krčmář wrote:
> 2017-05-23 23:24-0700, Nick Desaulniers:
> > +	fx_state = kmalloc(sizeof(*fx_state), GFP_KERNEL);
> > +	fx_state = kmalloc(sizeof(*fx_state), GFP_KERNEL);
> 
> fx_state must be 16 byte aligned and x86 ARCH_KMALLOC_MINALIGN is 8, so
> this needs manual correction.

Radim, thanks for the code review. I appreciate it! I'm fairly new to
kernel patching, but I'd like to resolve this with your guidance.

Looking through the available functions in slab.h, it seems the only
function that allows specifying alignment is kmem_cache_create.

Is that the simplest solution?

If so, then it seems like a `struct kmem_cache *` should be added as a
member to `struct x86_emulate_ctxt`? I'm not sure yet where the context
gets initialized and destroyed.  Do you have any insight?

Also, is there a #define'd value to use for the 16B alignment that I
should be using?

> Also, please kmalloc also fxregs_state in fxrstor_fixup and em_fxsave so
> we again have only one storage type.

Then it should be easy to call kmem_cache_alloc() at these sites.

> > +	if (!fx_state)
> > +		return -ENOMEM;
> 
> The caller does not understand -ENOMEM.  The appropriate return value is
> X86EMUL_UNHANDLEABLE.

Ack.

> >  	if (ctxt->mode < X86EMUL_MODE_PROT64)
> > -		rc = fxrstor_fixup(ctxt, &fx_state);
> > +		rc = fxrstor_fixup(ctxt, fx_state);
> 
> Ah, fxrstor_fixup most likely got inlined and both of them put ~512 byte
> fxregs_state on the stack ... noinline attribute should solve the
> warning too.

While that would change fewer lines, doesn't the problem still exist in
the case of `ctxt->mode < X86EMUL_MODE_PROT64`, just split across two
stack frames?  As in shouldn't we still dynamically allocate fx_state?