Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential
 computing secrets
To:     Dave Hansen <dave.hansen@intel.com>, linux-efi@vger.kernel.org
Cc:     Borislav Petkov <bp@suse.de>, Ashish Kalra <ashish.kalra@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andi Kleen <ak@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Andrew Scull <ascull@google.com>,
        "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
        James Bottomley <jejb@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@linux.ibm.com>,
        Jim Cadden <jcadden@ibm.com>,
        Daniele Buono <dbuono@linux.vnet.ibm.com>,
        linux-coco@lists.linux.dev, linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, Dov Murik <dovmurik@linux.ibm.com>
References: <20211007061838.1381129-1-dovmurik@linux.ibm.com>
 <20211007061838.1381129-5-dovmurik@linux.ibm.com>
 <290c21a8-a68f-0826-2754-1480f79a081d@intel.com>
From:   Dov Murik <dovmurik@linux.ibm.com>
Message-ID: <020a4e8b-20b6-15a0-13d3-66b232c221e3@linux.ibm.com>
Date:   Fri, 8 Oct 2021 08:51:55 +0300
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <290c21a8-a68f-0826-2754-1480f79a081d@intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

Thanks Dave for reviewing this.

On 07/10/2021 16:48, Dave Hansen wrote:
> On 10/6/21 11:18 PM, Dov Murik wrote:
>> +static void wipe_memory(void *addr, size_t size)
>> +{
>> +	memzero_explicit(addr, size);
>> +	clean_cache_range(addr, size);
>> +}
> 
> What's the purpose of the clean_cache_range()?  It's backed in a CLWB
> instruction on x86 which seems like an odd choice.  I guess the point is
> that the memzero_explicit() will overwrite the contents, but might have
> dirty lines in the cache.  The CLWB will ensure that the lines are
> actually written back to memory, clearing the secret out of memory.
> Without the CLWB, the secret might live in memory until the dirtied
> cachelines are written back.

Yes, that's the reason; as suggested by Andrew Scull in [1].

[1] https://lore.kernel.org/linux-coco/CADcWuH0mP+e6GxkUGN3ni_Yu0z8YTn-mo677obH+p-OFCL+wOQ@mail.gmail.com/

> 
> Could you document this, please?  It would also be nice to include some
> of this motivation in the patch that exports clean_cache_range() in the
> first place.
> 

Yes, I'll add that.


> I also think clean_cache_range() an odd choice.  If it were me, I
> probably would have just used the already-exported
> clflush_cache_range().  The practical difference between writing back
> and flushing the cachelines is basically zero.  The lines will never be
> reused.
> 

I agree that performance benefits of CLWB over CLFLUSH are negligible here
(but I have no way of measuring it).  Andrew suggested [2] that the extra
invalidation that CLFLUSH does it unnecessary.

But if we all agree that the clflush_cache_range() is OK here, I'm OK
with removing patch 1 and calling clflush_cache_range() in wipe_memory()
here.

Does anyone know of other locations in the kernel where memory is needed
to be scrubbed (zeroed and flushed) - like my wipe_memory()? Maybe there's
a standard way of doing this?


[2] https://lore.kernel.org/linux-coco/CADcWuH05vbFtJ1WYSs3d+_=TGzh-MitvAXp1__d1kGJJkvkWpQ@mail.gmail.com/


> *If* we export anything from x86 code, I think it should be something
> which is specific to the task at hand, like arch_invalidate_pmem() is.
> 
> Also, when you are modifying x86 code, including exports, it would be
> nice to include (all of) the x86 maintainers.  The relevant ones for
> this series would probably be:
> 
> X86 ARCHITECTURE (32-BIT AND 64-BIT)
> M:      Thomas Gleixner <tglx@linutronix.de>
> M:      Ingo Molnar <mingo@redhat.com>
> M:      Borislav Petkov <bp@alien8.de>
> M:      x86@kernel.org
> 
> X86 MM
> M:      Dave Hansen <dave.hansen@linux.intel.com>
> M:      Andy Lutomirski <luto@kernel.org>
> M:      Peter Zijlstra <peterz@infradead.org>
> 
> There's also the handy dandy scripts/get_maintainer.pl to help.
> 

You're right, sorry for missing it in this round.

But even if I remove the x86 change, I'll keep you copied anyway...


-Dov