With Bug 643342 (CVE-2017-5715) necessitating microcode updates for a subset of processors, installing and using the microcode should be as easy as possible. The old microcode wiki[1] instructed users to manually edit grub script files, and the new microcode wiki[2] is currently lacking details for manual early microcode loading. Instead of requiring manual intervention, grub should automatically find the microcode initrd file - especially now that sys-firmware/intel-microcode installs the initrd file directly into /boot/. There was a proposed patch[3] for similar, but more generic, behavior posted to the grub-devel list in 2016, but it never got any interest. [1] https://wiki.gentoo.org/wiki/Intel_microcode [2] https://wiki.gentoo.org/wiki/Microcode [3] http://lists.gnu.org/archive/html/grub-devel/2016-02/msg00025.html Reproducible: Always
Created attachment 515340 [details, diff] Patch for grub-2.02 adding microcode initrd support This adds code to /etc/grub.d/10_linux in order to detect some common microcode image file names. This behavior could be toggled behind a use flag to avoid duplicate behavior of Dracut or Genkernel. Though I have no idea if that would necessarily cause a problem.
Thanks, this was being discussed in some channels last week, but we were too busy to start on any implementation. 1. Rather than hardcoding the names only, let's introduce a variable, and set the DEFAULT content of the variable to the filenames: "intel-uc.img intel-ucode.img microcode.cpio" (there will probably be an amd-uc.img soon as well). 2. The upstream thread rejected it partially because the contributor introduced bashisms to a POSIX-only script. Needs to be changed to pure posix, so some simple replacements: - initrd+=" $i" + initrd="${initrd} $i" 3. I'm not sure if 'break' is the right construct in the loop to find it. Stuff you can put into an early initramfs(cpio): - ACPI tables - CPU microcode - Devicetable (DTB) - Crypto key material (discussed in upstream thread).
Created attachment 515778 [details, diff] [V2] Patch for grub-2.02 adding microcode initrd support
> 1. Rather than hardcoding the names only, let's introduce a variable, and > set the DEFAULT content of the variable to the filenames: "intel-uc.img > intel-ucode.img microcode.cpio" (there will probably be an amd-uc.img soon > as well). > 3. I'm not sure if 'break' is the right construct in the loop to find it. > Stuff you can put into an early initramfs(cpio): > - ACPI tables > - CPU microcode > - Devicetable (DTB) > - Crypto key material (discussed in upstream thread). The hard-coded list was an attempt to ensure that the microcode was loaded first, and that only one was loaded. I'm thinking of the case where a previously hand-installed image exists under a different name (I used /boot/microcode.cpio) now that intel-microcode automatically installs intel-uc.img. Maybe it would make sense to remove early*.cpio from the list, as I think most distros have settled on intel-*.img and amd-*.img, and leave it to the user to add it to GRUB_CUSTOM_INITRD_INCLUDES.
(In reply to Matthew Turnbull from comment #4) > > 3. I'm not sure if 'break' is the right construct in the loop to find it. > > Stuff you can put into an early initramfs(cpio): > > - ACPI tables > > - CPU microcode > > - Devicetable (DTB) > > - Crypto key material (discussed in upstream thread). > > The hard-coded list was an attempt to ensure that the microcode was loaded > first, and that only one was loaded. I'm thinking of the case where a > previously hand-installed image exists under a different name (I used > /boot/microcode.cpio) now that intel-microcode automatically installs > intel-uc.img. > > Maybe it would make sense to remove early*.cpio from the list, as I think > most distros have settled on intel-*.img and amd-*.img, and leave it to the > user to add it to GRUB_CUSTOM_INITRD_INCLUDES. This looks very promising now, I'm just worried that running it blindly on an system that might have either Intel _or_ AMD prefers the Intel microcode (instead of having both present and letting the kernel decide).
(In reply to Robin Johnson from comment #5) > (In reply to Matthew Turnbull from comment #4) > > > 3. I'm not sure if 'break' is the right construct in the loop to find it. > > > Stuff you can put into an early initramfs(cpio): > > > - ACPI tables > > > - CPU microcode > > > - Devicetable (DTB) > > > - Crypto key material (discussed in upstream thread). > > > > The hard-coded list was an attempt to ensure that the microcode was loaded > > first, and that only one was loaded. I'm thinking of the case where a > > previously hand-installed image exists under a different name (I used > > /boot/microcode.cpio) now that intel-microcode automatically installs > > intel-uc.img. > > > > Maybe it would make sense to remove early*.cpio from the list, as I think > > most distros have settled on intel-*.img and amd-*.img, and leave it to the > > user to add it to GRUB_CUSTOM_INITRD_INCLUDES. > This looks very promising now, I'm just worried that running it blindly on > an system that might have either Intel _or_ AMD prefers the Intel microcode > (instead of having both present and letting the kernel decide). I chatted to the other devs, drop the 'break' in the hardcoded loop, and we'd encourage users to clean up the old cpios in /boot.
Created attachment 515802 [details, diff] [V4] Patch for grub-2.02 adding microcode/custom initrd support Thanks for the feedback! That makes sense, especially from an upstream perspective, since that gives control and flexibility to distros so they can be platform agnostic in what they install.
It occurs to me, as I go back and review the old intel microcode docs, there is a comment about setting GRUB_DISABLE_LINUX_UUID=true if there is only a microcode image and not a true initrd. Would it be useful to bake that logic directly into 10_linux as well? It should be easy to do if we assume anything in GRUB_CUSTOM_INITRD_INCLUDES (rename to GRUB_CUSTOM_INITRD_EARLY_INCLUDES) are just for loading config/data/microcode/frmware files and aren't true initrd images.
Created attachment 515808 [details, diff] [V5] Patch for grub-2.02 adding microcode/custom initrd support + UUID handling Updated from V4 to disable UUID partition labels for early initrd only configurations. Renamed GRUB_CUSTOM_INITRD_INCLUDES -> GRUB_CUSTOM_INITRD_EARLY_INCLUDES.
Just to weigh-in as the package maintainer here: I would greatly prefer that this idea get accepted upstream and we apply it in Gentoo as a back-port. I don't want to be stuck maintaining a distro-specific patch for this several years from now.
(In reply to Matthew Turnbull from comment #9) > Created attachment 515808 [details, diff] [details, diff] > [V5] Patch for grub-2.02 adding microcode/custom initrd support + UUID > handling > > Updated from V4 to disable UUID partition labels for early initrd only > configurations. Renamed GRUB_CUSTOM_INITRD_INCLUDES -> > GRUB_CUSTOM_INITRD_EARLY_INCLUDES. This looks very promising. Minor changes: - renaming variables: - GRUB_CUSTOM_INITRD_INCLUDES -> GRUB_EARLY_INITRD_CUSTOM - add variable for the stock files default value # Common microcode filenames, including long & 8.3 naming : ${GRUB_EARLY_INITRD_STOCK:=intel-uc.img intel-ucode.img amd-uc.img amd-ucode.img early_ucode.cpio microcode.cpio} - change detection loop to be: $GRUB_EARLY_INITRD_STOCK $GRUB_EARLY_INITRD_CUSTOM - "initrd $(echo $initrd_path)" can't this just be "initrd ${initrd_path}"? - documentation: - cover both variables - include the crypto key example for GRUB_EARLY_INITRD_CUSTOM - signoff on patch should include both yourself and the original patch author from the upstream lists. I do agree w/ floppym that we need this submitted to upstream as well, but I'm willing to carry it while upstream debates it (email a v6 to them and link it from this bug).
Created attachment 516028 [details, diff] [V6] Patch for grub-2.02 adding early initrd support + UUID handling
(In reply to Robin Johnson from comment #11) > Minor changes: > - renaming variables: > - GRUB_CUSTOM_INITRD_INCLUDES -> GRUB_EARLY_INITRD_CUSTOM > - add variable for the stock files default value > # Common microcode filenames, including long & 8.3 naming > : ${GRUB_EARLY_INITRD_STOCK:=intel-uc.img intel-ucode.img amd-uc.img > amd-ucode.img early_ucode.cpio microcode.cpio} > - change detection loop to be: $GRUB_EARLY_INITRD_STOCK > $GRUB_EARLY_INITRD_CUSTOM Done. I wasn't sure where it would be most appropriate to set the default list, so I did it in grub-mkconfig.in before the configuration file is sourced. That should allow it to be inherited from the environment (i.e. /etc/env.d/) and overridden in /etc/default/grub. Also, I tweaked the rename to use GRUB_EARLY_INITRD_LINUX_CUSTOM and GRUB_EARLY_INITRD_LINUX_STOCK, since this is only implemented in 10_linux and I don't know if other OSes support [multiple] early initrd images. > - "initrd $(echo $initrd_path)" can't this just be "initrd ${initrd_path}"? I did this to strip off leading and trailing white space. It was bothering me when I was diffing test files, and the formatting of the generated grub.cfg looks very intentional. This seemed like a simpler solution than building it into the loop that constructs $initrd_path, or unnecessarily running it through sed. > - documentation: > - cover both variables > - include the crypto key example for GRUB_EARLY_INITRD_CUSTOM > - signoff on patch should include both yourself and the original patch > author from the upstream lists. Forgive a question about procedure, but is it necessary to get a sign-off from the old patch's author? Or is that more about not stepping on toes? I did admittedly copy a gettext_printf in earlier revisions, but even that has been stripped out and rewritten. What's the line between something being derivative, and being new? There's only so many different ways to write a for loop :) I only linked to the old patch to illustrate the previous lukewarm reception to a similar change, and no forward progress for 2 years. > I do agree w/ floppym that we need this submitted to upstream as well, but > I'm willing to carry it while upstream debates it (email a v6 to them and > link it from this bug). Sounds good. I started here given the afore mentioned upstream reception, and I was looking to get some feedback (which has been appreciated!) before engaging upstream (i.e. was this even a good idea). I will take care of that within the next day or two, pending any additional feedback.
BTW: Is this really needed? Looks like this should already work out of the box, not? https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=820838 / https://savannah.gnu.org/bugs/index.php?47681
(In reply to Matthew Turnbull from comment #13) > Also, I tweaked the rename to use GRUB_EARLY_INITRD_LINUX_CUSTOM and > GRUB_EARLY_INITRD_LINUX_STOCK, since this is only implemented in 10_linux > and I don't know if other OSes support [multiple] early initrd images. +1 on your naming choice and matching docs. > > - "initrd $(echo $initrd_path)" can't this just be "initrd ${initrd_path}"? ... > This seemed like a simpler solution than building it into the loop that > constructs $initrd_path, or unnecessarily running it through sed. I'm not fussed about whitespace, but you could be if you wanted. Vertical whitespace would be a good reason to keep the echo. > > - signoff on patch should include both yourself and the original patch > > author from the upstream lists. ... > I only linked to the old patch to illustrate the previous lukewarm reception > to a similar change, and no forward progress for 2 years. I mean it not as DCO, but at least noting it was based on their patch. Credits: original-author > > I do agree w/ floppym that we need this submitted to upstream as well, but > > I'm willing to carry it while upstream debates it (email a v6 to them and > > link it from this bug). > > Sounds good. I started here given the afore mentioned upstream reception, > and I was looking to get some feedback (which has been appreciated!) before > engaging upstream (i.e. was this even a good idea). > > I will take care of that within the next day or two, pending any additional > feedback. No more feedback from me on the patch. In your submission to upstream, you're going to want a good commit message to go with it. You should reference the original mails, and explicitly note that you addressed the concerns there (the cryptokey, the bashisms) For v6 submission, you can put: Signed-Off-By: Robin H. Johnson <robbat2@gentoo.org> Floppym: If you don't see any other issues with the patch, can we start carrying it in the Gentoo package shortly? I'm willing to write up the news item to go with it, detailing the USE=initramfs on intel-microcode that will consume this for Meltdown/Spectre.
(In reply to Thomas Deutschmann from comment #14) > BTW: Is this really needed? Looks like this should already work out of the > box, not? https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=820838 / > https://savannah.gnu.org/bugs/index.php?47681 Those bugs are for sys-boot/os-prober, which isn't relevant in this case specific case. They do need fixing, but aren't a holdup in this case.
Apologies that it took me so long to get this sent upstream. Life/work got very hectic very suddenly. http://lists.gnu.org/archive/html/grub-devel/2018-02/msg00112.html
Well done guys: http://git.savannah.gnu.org/cgit/grub.git/commit/?id=a698240df0c43278b2d1d7259c8e7a6926c63112
The bug has been closed via the following commit(s): https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=4ce63f8b85aa62e485eaebc34b36024f80866106 commit 4ce63f8b85aa62e485eaebc34b36024f80866106 Author: Mike Gilbert <floppym@gentoo.org> AuthorDate: 2018-04-01 18:17:04 +0000 Commit: Mike Gilbert <floppym@gentoo.org> CommitDate: 2018-04-01 18:18:14 +0000 sys-boot/grub: backport early microcode patch Closes: https://bugs.gentoo.org/645088 Package-Manager: Portage-2.3.24, Repoman-2.3.6_p81 .../grub/files/2.02-multiple-early-initrd.patch | 177 ++++++++++++ sys-boot/grub/grub-2.02-r1.ebuild | 299 +++++++++++++++++++++ 2 files changed, 476 insertions(+)
