Mattiwatti
commented
9 months ago Hi, thanks for your contribution.
A general comment re: your PR: I don't love compile time defines, and your PR adds 3. I know that EfiGuard already has CONFIGURE_DRIVER currently, but I sometimes wonder if adding this as a build time option wasn't a mistake compared to some of the alternatives such as having a configuration text file or command line arguments, or just removing the interactive loader altogether (I've never had a single report from anyone using it - but maybe people do use it and it just works well? I don't know.) Right now, CONFIGURE_DRIVER requires me to add an additional build target to each release ZIP, which makes releases more confusing for new users (what's this extra file for? Do I need it? Etc.) Any added boolean switch on top of this would require a further 2x the current number of build targets.
Other than this, there are a few ideas here, so I will address them individually:
-
DO_NOT_DISABLE_PATCHGUARD: this is not OK. Reasons for this:- EfiGuard's purpose is to disable PatchGuard. Anything else it does or can do such as disabling DSE is only really there for convenience and ease of use.
- Leaving PatchGuard initialized will essentially make any use of the runtime
SetVariablehook to disable DSE a sort of delayed bugcheck, due to PatchGuard.
Note that I'm aware that you don't need to disable DSE after booting with EfiGuard, so (2) may not apply if you have a use case for this that I can't see at the moment. But that still leaves (1), so this use case had better be a good one. If you want EfiGuard to not disable PatchGuard, and at runtime to disable DSE, you will run into a PatchGuard bugcheck as noted above. If you want EfiGuard to neither disable PatchGuard nor DSE... what are you using it for?
EFIGUARD_DRIVER_FILENAME: what is the use case for this? I'm not necessarily opposed to this, but I also don't think it's very useful. Is it not possible to make this a runtime switch?EAC_COMPAT_MODE: this addresses a real issue so it is therefore definitely valid in that sense. However:- (minor issue) Not patching
KiMcaDeferredRecoveryServiceisn't required to do what you are trying to do here as far as I can tell, nor is it safe. This patch should be left enabled. - Enabling this option will make EfiGuard incompatible with Riot Vanguard, or more generally any kernel code that issues
int 20h. Any system with Riot Vanguard installed will bugcheck during boot ifKiSwInterruptDispatchis allowed to dereference a null PG context. (This is the reason theKiSwInterruptpatch was added in the first place.) - Additionally, adding this as a build time switch causes all of the issues already mentioned above, which means that my question would be: is this important enough to warrant the additional complexity? For me as someone who doesn't play video games, the answer is obviously no. However, your PR got me interested in taking a look at this issue, and I believe it's probably possible to fix it (for now - you should obviously expect this to be un-fixed by EAC again at some point) in a way that retains compatibility with Vanguard while also not requiring a new build time switch. It would also be preferable to the current
KiSwInterruptpatch regardless of anything EAC does either way. I don't have an ETA for this fix but I do intend to add it for the next release.
- (minor issue) Not patching
So in conclusion, I won't be accepting this. I do want to say thanks regardless for bringing the EAC issue to my attention, especially in a way that gave me enough to go on to reproduce it and narrow down potential causes myself.
utoni
Not sure if that may be of any use for anyone. But just in case, here is the PR.