Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets

[Dave Hansen]

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.

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.

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.

*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@xxxxxxxxxxxxx>
M:      Ingo Molnar <mingo@xxxxxxxxxx>
M:      Borislav Petkov <bp@xxxxxxxxx>
M:      x86@xxxxxxxxxx

X86 MM
M:      Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
M:      Andy Lutomirski <luto@xxxxxxxxxx>
M:      Peter Zijlstra <peterz@xxxxxxxxxxxxx>

There's also the handy dandy scripts/get_maintainer.pl to help.