Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

dannf
Copy link
Contributor

@dannf dannf commented Aug 19, 2022

…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

frozencemetery
frozencemetery requested changes Aug 19, 2022
View reviewed changes
Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

frozencemetery
frozencemetery requested changes Aug 19, 2022
View reviewed changes
pe.c
@@ -1196,6 +1197,9 @@ handle_image (void *data, unsigned int datasize,

CopyMem(buffer, data, context.SizeOfHeaders);

/* Flush the instruction cache for the region holding the image */
cache_invalidate(buffer, buffer + context.ImageSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line and the line above suggest to me that the invalidation always occurs, while that's not what the function does.

Copy link
Contributor Author

@dannf dannf Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @frozencemetery. Along with correcting the comment, what do you think about renaming the function maybe_cache_invalidate() or perhaps arch_cache_invalidate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a version w/ the arch_cache_invalidate() name, but open to other suggestions.

@dannf dannf changed the title shim: Flush the executable's memory region from i-cache before execut… shim: Flush the memory region from i-cache before execution Aug 22, 2022
@dannf
Copy link
Contributor Author

dannf commented Aug 22, 2022

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 dannf mentioned this pull request Aug 25, 2022
Merged
@dannf
shim: Flush the memory region from i-cache before execution
812ad99 
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 has been
shown to avoid the problem, which has parallels with this story:
  https://www.mail-archive.com/osv-dev@googlegroups.com/msg06203.html

Add a cache flushing utility function and provide an implementation
using a GCC intrinsic. This will need to be extended to support other
compilers. Note that this intrinsic is a no-op for x86 platforms.

This fixes issue rhboot#498.

Signed-off-by: dann frazier <dann.frazier@canonical.com>
@dannf
Copy link
Contributor Author

dannf commented Sep 7, 2022

I've pushed a new version due to feedback at rhboot/grub2#107 (comment)

  • 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
Copy link
Contributor

vathpela commented Oct 4, 2022

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

@vathpela vathpela closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frozencemetery frozencemetery frozencemetery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dannf @vathpela @frozencemetery