-
Notifications
You must be signed in to change notification settings - Fork 127
Add scripts that sign the module for Secure Boot #563
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
base: master
Are you sure you want to change the base?
Conversation
Thanks, this look useful. I'll first let Copilot do a review. I'll look into this soon, I hope. I'm currently lacking some time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for signing kernel modules for Secure Boot compatibility by introducing two automated scripts. The scripts handle the complete workflow of generating signing keys, importing them into the MOK (Machine Owner Key) system, and signing the hid-xpadneo kernel module.
Key changes:
- Added automated signing and installation scripts for both DKMS and generic installations
- Implemented MOK key generation and certificate management for Secure Boot
- Added documentation explaining how to use the signing scripts
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
File | Description |
---|---|
sign_and_install.sh | DKMS-specific script that automates key generation, MOK enrollment, and module signing for Secure Boot |
hid-xpadneo/sign_and_install_generic.sh | Generic installation script with similar functionality but uses direct make commands instead of DKMS |
docs/README.md | Documentation update explaining how to use the new signing scripts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I addressed Copilot feedback and did some small improvements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicks. I appreciate your work but I have to admit that I did just a quick scan through your changes and didn't test it yet. That's because I don't run a secure boot system here. I'd need to setup a VM or container - probably with different distributions.
If that works, tho, I'm pretty confident I can use that as a template to make my bare metal system secureboot. So, thanks already here. :-)
I am no expert on this topic so I am not really sure about anything to be honest. I did not do a ton of research. The installation failed for me so I adapted a script that I found for another project and figured that it could be useful for other people too |
Given the discussion, I'm guessing this script is mainly used for manual installation, not for the module being managed by dkms? Tho, your doc patches say "if you use dkms...". I think we should check how other dkms-managed modules handle that. |
It turns out I was doing it wrong. I was signing AFTER dkms has installed the module, using custom MOK, which prevented auto rebuilds. But in the build output, there is these lines: Sign command: /usr/bin/kmodsign
Signing key: /var/lib/shim-signed/mok/MOK.priv Indeed Signing module /var/lib/dkms/hid-xpadneo/v0.9-228-g2d516d3-dirty/build/src/hid-xpadneo.ko |
Sounds promising, I really appreciate it but will need to find some spare time to test it in a VM. |
I simplified the script and improved the process again, by using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good cleanup but there are some issues.
Makefile
Outdated
echo "Skipping key creation" | ||
endif | ||
@$(KSRC)/scripts/sign-file sha256 MOK.priv MOK.der hid-xpadneo.ko | ||
@sudo update-secureboot-policy --new-key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Makefile should not call sudo, especially it should not do so hidden, and it should not run by surprise (e.g., you may have unlocked sudo before, and then it would run without asking for passwords). This also makes no sense because modprobe
requires sudo but doesn't use it here. I'd suggest to rather add a guard [ "$(id -u)" = "0" ] || { echo >&2 "need to run with sudo"; exit 1; }
.
There are exceptions to this rule but this one is obviously not it.
sign_and_install.sh
Outdated
else | ||
if mokutil --sb-state | grep -q "SecureBoot enabled" | ||
then if ! grep -q "hid_xpadneo" /proc/modules && info "hid_xpadneo is loaded" | ||
then if ! grep -q "hid_xpadneo" /proc/modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail on systems which linked xpadneo statically into the kernel. Check for [ -d /sys/module/hid_xpadneo ]
, it will tell you if the driver is present in the kernel. OTOH, if xpadneo is statically linked into the kernel, you won't have to try signing it in the first place. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now look into /sys/module/hid_xpadneo
. I will try to to add a check to see if the module should not be signed because it is statically linked into the kernel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modinfo will return (builtin)
on the filename line if it is builtin, e.g. I have btrfs built into the kernel:
# modinfo btrfs
name: btrfs
filename: (builtin)
...
But if a module is unknown, it will return an error:
# modinfo unknown || echo "exit code: $?"
modinfo: ERROR: Module unknown not found.
exit code: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I think about it, I do not think I need to worry about that, since, as you said, there is no need to sign it if the module is statically linked. And I already skip the signing & install when the module is loaded (as it should be if statically linked I guess)
Makefile
Outdated
@$(KSRC)/scripts/sign-file sha256 $(SHIM_MOK_DIR)/MOK.priv $(SHIM_MOK_DIR)/MOK.der $(LIB_PREFIX)/hid-xpadneo.ko | ||
modprobe hid-xpadneo | ||
|
||
sign-install: all install sign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pull in all
here, install
already pulls build
.
Makefile
Outdated
@sudo update-secureboot-policy --new-key | ||
@sudo update-secureboot-policy --enroll-key | ||
@$(KSRC)/scripts/sign-file sha256 $(SHIM_MOK_DIR)/MOK.priv $(SHIM_MOK_DIR)/MOK.der $(LIB_PREFIX)/hid-xpadneo.ko | ||
modprobe hid-xpadneo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recipe says "sign" - don't modprobe. Especially, modprobe
won't reload the module, so the outcome is not deterministic.
Makefile
Outdated
rm -f $(MODPROBE_CONFS:%=$(PREFIX)$(ETC_PREFIX)/modprobe.d/%) | ||
rmdir --ignore-fail-on-non-empty -p $(PREFIX)$(ETC_PREFIX)/modprobe.d $(PREFIX)$(ETC_PREFIX)/udev/rules.d $(PREFIX)$(DOC_PREFIX) | ||
|
||
sign: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need $(LIB_PREFIX)/hid-xpadneo.ko
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need $(SHIM_MOK_DIR)/MOK.priv $(SHIM_MOK_DIR)/MOK.der
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if signing with the Makefile
is even worth it. It should be handle by dkms. If you need this for development, maybe move it to hid-xpadneo/Makefile
instead? That file also allows usage of sudo for developer convenience. I'm using that to rebuild and reload my module during development like this:
make -C hid-xpadneo reinstall
Makefile
Outdated
rm -f $(MODPROBE_CONFS:%=$(PREFIX)$(ETC_PREFIX)/modprobe.d/%) | ||
rmdir --ignore-fail-on-non-empty -p $(PREFIX)$(ETC_PREFIX)/modprobe.d $(PREFIX)$(ETC_PREFIX)/udev/rules.d $(PREFIX)$(DOC_PREFIX) | ||
|
||
sign: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need $(SHIM_MOK_DIR)/MOK.priv $(SHIM_MOK_DIR)/MOK.der
...
Makefile
Outdated
sign: | ||
@sudo update-secureboot-policy --new-key | ||
@sudo update-secureboot-policy --enroll-key | ||
@$(KSRC)/scripts/sign-file sha256 $(SHIM_MOK_DIR)/MOK.priv $(SHIM_MOK_DIR)/MOK.der $(LIB_PREFIX)/hid-xpadneo.ko |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add the dependency for the sign recipe, you can use $<
to reference the first dependency aka $(LIB_PREFIX)/hid-xpadneo.ko
.
…dule for Secure Boot
73fd8cd
to
6ef9bf6
Compare
`dkms remove` may fail because a previous installation has been broken or tampered with. This will cause the remaining uninstallation to fail. Fix this by logging that we proceed anyways. Reported-by: @flodavid Link: atar-axis#563 Signed-off-by: Kai Krakow <[email protected]>
`dkms remove` may fail because a previous installation has been broken or tampered with. This will cause the remaining uninstallation to fail. Fix this by logging that we proceed anyways. Reported-by: flodavid <[email protected]> Link: atar-axis#563 Signed-off-by: Kai Krakow <[email protected]>
Thank you for your feedback and advices, that’s appreciated. If think the I am still working on the generic install. I am not sure about the use of the PEM certificate and its password. I will also try to have good signing rules in the makefile and maybe remove some bash functions to use those instead. I have a problem on my system though. I guess I have firmware 5.x since the module is not working without |
`dkms remove` may fail because a previous installation has been broken or tampered with. This will cause the remaining uninstallation to fail. Fix this by logging that we proceed anyways. Suggested-by: flodavid <[email protected]> Link: atar-axis#563 Signed-off-by: Kai Krakow <[email protected]>
I'm not sure how we should properly handle that. It is expected that bluez loads uhid if it is needed. xpadneo is not responsible to do that, we are a HID user, not a HID provider. So I think you should report it to your distribution, maybe they should link uhid statically into the kernel, or add a dependency. Until then, you could just run: # always load uhid on boot (requires systemd)
echo "uhid" | sudo tee -a /etc/modules-load.d/99-local.conf
# OR: load uhid with xpadneo (untested)
echo "softdep hid-xpadneo pre: uhid" | sudo tee -a /etc/modprobe.d/99-xpadneo-local.conf
# ALTERNATIVE: load uhid with bluetooth (untested, probably preferred)
echo "softdep bluetooth pre: uhid" | sudo tee -a /etc/modprobe.d/99-bluetooth-uhid.conf If uhid-related reports keep popping up, we might need to add the latter into xpadneo, tho, I still think, distributions should add that to the standard kernel if they provide bluez with BLE support (or make the bluez service load the module). BLE input devices cannot be used without uhid. Thus, there's no point in adding a softdep to each driver which in theory might maybe support a Bluetooth HID device... EDIT: I added another alternative. |
Thanks. I tried What's strange is that I have no vibration when the controller is connected, even though there is |
This can happen if xpadneo needs to bind lazily to the device via udev rules - it may not have worked during the first connection, and then only on second connect, it will work (and that means, also You should see a difference in dmesg for both tries - if you can still replicate it...
Which proves that the driver providing the HID bus has to load it - which is bluez. xpadneo is only a HID consumer. It doesn't even know about BT. Most likely, Actually, there's a race condition with If you asked me, |
The system should trigger loading of the `uhid` module when `bluetooth` is loaded to properly support BLE devices which provide the HID bus from user-space through bluez. This may still leave a gap for a race-condition if both `bluetooth` and the BLE device appear at the same time. `uhid` should be present before any device even connects from user-space, otherwise `hid-xpadneo` may not see the HID bus early enough. I'm unsure if we also should provide `weakdep` to trigger inclusion of `uhid` into initrd - but since Xbox controllers are not critical input devices during early boot, this is probably not our business. OTOH, `softdep` may already trigger inclusion on its own (untested). Link: atar-axis#563 (comment) Signed-off-by: Kai Krakow <[email protected]>
The system should trigger loading of the `uhid` module when `bluetooth` is loaded to properly support BLE devices which provide the HID bus from user-space through bluez. This may still leave a gap for a race-condition if both `bluetooth` and the BLE device appear at the same time. `uhid` should be present before any device even connects from user-space, otherwise `hid-xpadneo` may not see the HID bus early enough. I'm unsure if we also should provide `weakdep` to trigger inclusion of `uhid` into initrd - but since Xbox controllers are not critical input devices during early boot, this is probably not our business. OTOH, `softdep` may already trigger inclusion on its own (untested). Hint: This feels wrong because xpadneo is the HID consumer, not the HID provider. We should not need to do that. But it prevents bug reports from users in the future. Link: atar-axis#563 (comment) Signed-off-by: Kai Krakow <[email protected]>
The system should trigger loading of the `uhid` module when `bluetooth` is loaded to properly support BLE devices which provide the HID bus from user-space through bluez. This may still leave a gap for a race-condition if both `bluetooth` and the BLE device appear at the same time. `uhid` should be present before any device even connects from user-space, otherwise `hid-xpadneo` may not see the HID bus early enough. I'm unsure if we also should provide `weakdep` to trigger inclusion of `uhid` into initrd - but since Xbox controllers are not critical input devices during early boot, this is probably not our business. OTOH, `softdep` may already trigger inclusion on its own (untested). Hint: This feels wrong because xpadneo is the HID consumer, not the HID provider. We should not need to do that. But it prevents bug reports from users in the future. Tested-by: flodavid <[email protected]> Link: atar-axis#563 (comment) Signed-off-by: Kai Krakow <[email protected]>
938f39b
to
8c68f33
Compare
I created scripts to automate the signing and installation steps required on systems with Secure Boot enabled