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.

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

kvm/x86: Add pristine ECTX check #897

Closed

Conversation

mschlumpp
Copy link
Member

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.pl on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): x86_64
  • Platform(s): kvm
  • Application(s): N/A

Additional configuration

  • CONFIG_UKPLAT_ISR_ECTX_ASSERTIONS=y

Description of changes

Currently, interrupt handlers could silently modify the values of extended context registers. More recent compilers are more likely to generate vectorized machine code that utilizes these registers. This happened in the past and usually caused hard to find bugs.

This pull request implements a check on x86/kvm, but can be extended for other architectures/platforms.

@mschlumpp mschlumpp requested review from a team as code owners May 17, 2023 07:08
@razvand razvand requested review from razvanvirtan and rares-miculescu and removed request for a team May 17, 2023 07:12
@razvand razvand added this to the v0.14.0 (Prometheus) milestone May 17, 2023
@razvand razvand added area/lib Internal Unikraft Microlibrary plat/kvm Unikraft for KVM lang/c Issues or PRs to do with C/C++ arch/x86_64 labels May 17, 2023
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
d3ed251 arch/x86: Add function to check for unmodified ectx
6498935 plat/kvm: Add ectx assertion in interrupt handler

@unikraft-bot unikraft-bot added area/arch Unikraft Architecture area/include Part of include/uk area/plat Unikraft Patform labels May 17, 2023
@skuenzer skuenzer self-assigned this May 23, 2023
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Thanks, @mschlumpp. I tested it, it works. I looked through the code and commits. Everything is OK.

Reviewed-by: Razvan Deaconescu razvand@unikraft.io

Copy link

@rares-miculescu rares-miculescu left a comment

Choose a reason for hiding this comment

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

I tested it and it works here too. Thanks!

Reviewed-by: Rares Miculescu miculescur@gmail.com

This can be used in situtations where the executed code must not touch
the extended registers. For example, interrupt handlers in Unikraft do
save these and therefore also cannot modify them. Doing so is usually
breaking the interrupted code.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
This checks that the extended registers were not modified within the
interrupt service routine. This is not free and therefore can be
toggled via a KConfig option.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
@mschlumpp mschlumpp force-pushed the mschlumpp/feature/ectx-assertion branch from 6498935 to e8c8342 Compare June 21, 2023 15:25
@razvand razvand added the size/small Small PR, quick to review label Jun 28, 2023
@razvand razvand requested a review from skuenzer August 9, 2023 10:25
Copy link
Member

@skuenzer skuenzer left a comment

Choose a reason for hiding this comment

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

Approved-by: Simon Kuenzer simon@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 10, 2023
This checks that the extended registers were not modified within the
interrupt service routine. This is not free and therefore can be
toggled via a KConfig option.

Signed-off-by: Marco Schlumpp <marco@unikraft.io>
Reviewed-by: Razvan Deaconescu <razvand@unikraft.io>
Approved-by: Simon Kuenzer <simon@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #897
@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 10, 2023
@mschlumpp mschlumpp deleted the mschlumpp/feature/ectx-assertion branch August 10, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch/x86_64 area/arch Unikraft Architecture area/include Part of include/uk area/lib Internal Unikraft Microlibrary area/plat Unikraft Patform ci/merged Merged by CI lang/c Issues or PRs to do with C/C++ plat/kvm Unikraft for KVM size/small Small PR, quick to review
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

6 participants