Messages in this thread | | | Subject | Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets | From | Dov Murik <> | Date | Fri, 8 Oct 2021 08:51:55 +0300 |
| |
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
| |