linux-kernel - Re: [PATCH v7 0/5] x86: Fix SEV guest regression

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <alpine.DEB.2.21.1809132042100.1473@nanos.tec.linutronix.de>
Date:   Thu, 13 Sep 2018 20:47:33 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Brijesh Singh <brijesh.singh@....com>
cc:     x86@...nel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Tom Lendacky <thomas.lendacky@....com>,
        Borislav Petkov <bp@...e.de>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Radim Krčmář <rkrcmar@...hat.com>
Subject: Re: [PATCH v7 0/5] x86: Fix SEV guest regression

On Thu, 13 Sep 2018, Brijesh Singh wrote:
> On 09/13/2018 11:22 AM, Thomas Gleixner wrote:
> > I really have to ask why this whole change to sme_encrypt_kernel() is
> > required at all.
> 
> The main reason behind making the changes in sme_encrypt_kernel() was
> to perverse the initialized values (if any). Since we are adding
>  __decrypted attribute hence I thought it would make sense to support
> the usecase where in future someone may use this attribute on initialized
> variables.
> 
> Now the question is, do we really need this to fix the regression? the
> answer is NO. Since there is only one user of this new attribute and
> lucky it does not initialize the variables hence it is safe to move the
> variable in .bss..decrypted section and memset(0).

And even if it would it usually trivial enought to make it run time
initialized.

> > 1) Add a .bss..decrypted section which is PMD aligned and mark the
> >     kvmclock hv_clock_boot and wallclock struct with __bss_decrypted.
> >  > 2) Fixup the .bss..decrypted section PMD in the SEV case at the end of
> >     sme_encrypt_kernel() and do a memset(0) on that. That does not require
> >     any of the restructuring, really.
> > 
> 
> Somewhere during the discussion, I was asked to make sure that
>  __decrypted attribute can be used by others in future and don't tie it
> with just the kvmclock.

Well, .bss..decrypted can be used by other things and we really only want
to do the .data..decrypted thing when there is a fundamental reason to do
so.

> To fix the regression we don't need to have this complexity. I am okay
> to implement your proposal. I would like to fix this regression sooner
> than later ;)
> 
> 
> > 3) Have a function which is called after the page allocator is up which
> >     does:
> > 
> 
> I am glad you are pointing this one. In my initial patch I was doing a
> kmalloc() of page-size, during review we did discussed to do allocation
> once using num_possible_cpus()[like what you have proposed]. But then
> discussion moved towards saving memory and that when the secondary array
> concept came into the picture. Since .data..decrypted has some unused memory
> hence we were getting creative in reusing it.

Sorry I didn't pay attention to that, was busy with other urgent stuff.

> > That should be a halfways slim sized and non instrusive changeset.
> > 
> 
> I am okay to implement and test your recommendation, if anyone
> disagree then please let me know. thanks

It results in simpler code and if the whole thing ends up allocating a few
KB too much due to the page allocations then so be it. It's not going to
waste 256K in one go.

Thanks,

	tglx