shim: Flush the memory region from i-cache before execution

rhboot / shim

UEFI shim loader

Other

848 stars 290 forks source link

shim: Flush the memory region from i-cache before execution #504

Closed dannf closed 1 year ago

dannf commented 2 years ago

…ing it

We've seen crashes in early GRUB code on an ARM Cortex-A72-based platform that point at seemingly harmless instructions. Flushing the i-cache of those instructions prior to executing seems to work fine, which seems to have parallels with this story: https://www.mail-archive.com/osv-dev@googlegroups.com/msg06203.html

Add a cache flushing utility function that is a no-op for !arm64, and and provide an implementation using a GCC intrinsic.

This fixes issue #498.

Signed-off-by: dann frazier dann.frazier@canonical.com

dannf commented 2 years ago

Thanks for the patch. I've a comment inline... since I'm asking for changes already, please also fixup the line lengths in your commit messages (I know we're not always great about this, but it's nicer to have 72 in body and not much over 50 in subject).

Also done, thanks for the feedback!

dannf commented 2 years ago

I've pushed a new version due to feedback at https://github.com/rhboot/grub2/pull/107#issuecomment-1232205900

It no longer has an architecture guard; we rely on the compiler to decide if __builtin___clear_cache() should be a no-op on that architecture (GCC does so on x86)
I renamed arch_cache_invalidate back to cache_invalidate because we now compile it everywhere - the compiler just may decide it doesn't need to generate code for it.

fyi, I tested with clang as well, and found that it does define __GNUC__, and does apparently implement __builtin___clear_cache(), though shim fails to build with clang for other reasons.

vathpela commented 1 year ago

I've pushed this as 5c537b3d0cf8c393dad2e61d49aade68f3af1401, which is just organized a bit differently.