Thanks Dave for reviewing this.

On 07/10/2021 16:48, Dave Hansen wrote:

On 10/6/21 11:18 PM, Dov Murik wrote:

quoted

+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/ (local)

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/ (local)

*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 [off-list ref]
M:      Ingo Molnar [off-list ref]
M:      Borislav Petkov [off-list ref]
M:      x86@kernel.org

X86 MM
M:      Dave Hansen [off-list ref]
M:      Andy Lutomirski [off-list ref]
M:      Peter Zijlstra [off-list ref]

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