GbE-region MAC randomisation - #2171
Conversation
|
Confused on how ifdtool is built here vs #1195 (which was unclean still, required coreboot's built ifdtool that is needed to build coreboot (see modules/coreboot there) to add Makefile hacks to pack ifdtool as cbmem is packed to be used inside of Heads. ifdtool needs to be built with musl-cross-make to depend on musl's libc; so unclear how building it with coreboot's buildstack produces an actual working ifdtool to be used inside of heads? You tested this pr @FckBigTch ? |
Yes, I've tested several times, and flashed the resulting ROMs on t480s, t480 and x230. Ifdtool is usable inside of heads. Confirmed it's actually musl: Interpreter is /lib/ld-musl-x86_64.so.1 |
b04ba37 to
931e455
Compare
|
Some notes from deeper review
Will think about that a bit more. If you have ideas let me know @FckBigTch |
The reason I thought the Novacustom models should be excluded is that the config explicitly states: That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update. I hadn't even thought of the idea with the dynamic menu. I'll follow your advice and try to implement it. |
Misunderstanding. This means those boards pack a GBE (see coreboot configs) but don't ever flash it per initial flashing instructions or internal flashing. If someone misread initial flashing (which is done by OEM not end users in the case of these boards), then MAC address would become generic as per packed in rom GBE.
|
Also see linuxboot/heads-wiki#209 (needed) |
fc5e070 to
03a9a5a
Compare
|
I've revised the menu now. Instead of duplicating the whole function for the gated/non-gated case, the menu items are now built into a bash array, with the MAC randomization entry conditionally appended only when CONFIG_NVMUTIL=y and CONFIG_IFDTOOL=y. |
|
Took a little time to think about linuxboot/heads-wiki#209 and current widespread
It seems that the proper usage pattern would become, in all boards having GBE
Thoughts @FckBigTch ? |
Sorry i misled you @FckBigTch So, what that means is that the SPI chip HAS a GBE, but shipped ROM images don't and keep OEM's. So TLDR: We should have all boards having GBE only write to specific IFD's regions, not touching GBE even if one is provided by rom (excluding GBE from flashprog). And have GBE flashed when user intends it (when one is generated from nvmutil in this PR), where internal upgrade don't touch it unless expressively asked for. Makes sense? Outside of v540tu/v560tu not having GBE in packed ROM; Others have it: Makes sense? |
I'm sorry for the late reply, but I had to think about what you wrote. If the user or whoever first did the (external) flash overwrote the GBE, so it’s DE:AD:CO:FF:EE. If they use the randomization feature, they get a random MAC. If all boards exclude GBE (CONFIG_FLASH_OPTIONS change), the random mac is persistent during an update. I'm not sure if that's really necessary, but I understand that you want to offer an easy way to revert to the default settings. |
So, is there anything else you'd like me to do? |
Sorry for having confused you. I checked the changes, and now I understand why the misunderstandings. Once this is CONFIG_FLASH_OPTIONS and flash.sh used, why I wanted to keep prior CONFIG_FLASH_OPTIONS to restore it before returning to prior operations is exposed, otherwise a reboot.sh would be needed (also normal pattern after a flash. We might need to simply add in all boards CONFIG_FLASH_TOOL="flashprog" and CONFIG_FLASH_OPTIONS=$CONFIG_FLASH_TOOL --programmer....." so that you could use CONFIG_FLASH directly if that is the path you prefer without needing backup of previous board, extract the programmer to call only --ifd -i gbe... That's where I was going to. gui-init.sh should not have additional functions, it should either be in functions.sh or gui_functions.sh that gui-init.sh sources. One cannot cd into /tmp in one function and not change back to prior location either since other heads workflows will break, unless a reboot is done too there. Some of your added functions do not check for ifdtool or nvutil presence (not guarded) which might also cause issues down the line which we try to not have to deal with. Sorry I wasn't clear enough; I didn't see you were calling flashprog directly in added functions under gui-init.sh |
|
Make sense! Thanks for your patience and explanations. I will make the changes. |
03a9a5a to
87e7fbd
Compare
|
I've made the changes. I set the t420 and x220 to "no" in the Tools section of the config because of the space issue (7 MB). I'm not sure if the new CONFIG_FLASH_OPTIONS still make sense for these boards. Maybe it would be better to stick with the old version without options after all? |
yes, it should be unified everywhere (all boards) so we can clearly say in heads-wiki that we did a philosophical stance change; the user is left with a choice, per our discussions here, to flash externally with or without board's rom GBE, or, to keep the one that was fused in config per OEM/ODM. If we do so, the same general guidelines will be possible for all boards under https://osresearch.net/Flashing-guides (and we will be able to get rid of all those different flashing instructions which are basically duplicates with a generic "FLASHTOOL --progress --programmer XYZ [--chip ZYX] --ifd -i bios -i me -i fd" where user is invited to check for external programmer choice quirks without having to repeat ourselves. Makes sense? |
|
@FckBigTch I asked for testing/comments under matrix channel Please update OP to fit content. Ideally add screenshots :) |
|
Kano was merged. If you beat me to add, adding the same sperarion of flash prog and flash options to all boards would be necessary before merge. |
- nvmutil from libreboot added as a module - ifdtool from coreboot is included as a part of the Makefile Signed-off-by: Rene <chaotic@disroot.org>
- Ethernet MAC address randomization - menu option is displayed only if $CONFIG_NVMUTIL=y and $CONFIG_IFDTOOL=y are set in the config file - add mac_randomization_options_menu() - to select a completely random mac, an intel pattern or to show the current mac Signed-off-by: Rene <chaotic@disroot.org>
9a439ad to
39377ec
Compare
Have you checked the matrix thread? |
|
Tested on a W530. LGTM |
|
There is no reason waiting longer. Let's keep an eye open on bug reports and have that merged. Asking copilot for review one last time |
There was a problem hiding this comment.
🟡 Changes recommended
Unhandled nvmutil failures, unsafe globally administered random addresses, and unsupported Kano enablement must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 40/42 changed files
- Comments generated: 5
- Review effort level: Balanced
39377ec to
ffdb947
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate nvm invocations still mutate or read the GbE image before the checked calls, and the current-MAC dialog is shown before validating that a MAC was parsed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 40/42 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
LGTM let's see if copilot has something relevant for final review |
- add show_mac - add change_mac - add clean_up_mac, remove temporary files - adjust CONFIG_FLASH_OPTIONS in flash.sh, for mac randomization Signed-off-by: Rene <chaotic@disroot.org>
- add $CONFIG_NVMUTIL - add $CONFIG_IFDTOOL - change $CONFIG_FLASH_OPTIONS to "flashprog --progress --programmer internal --ifd -i bios -i me -i fd" for all boards have GBE packed in ROM - t420 and x220, tools are set to no, does not fit in 7mb image (like dropbear) Signed-off-by: Rene <chaotic@disroot.org>
ffdb947 to
050e5b8
Compare
|
I got my hopes up too soon, careless mistake. Now it should be fine |
|
@FckBigTch Hmm, retested on v540tu and to be flashed mac was the old one when selecting intel random one See
Specifically
I would recommend adding more DO_WITH_DEBUG and DEBUG calls to sink output to /tmp/debug.log. Will try to debug further later on, but it seems that the old mac is used instead of new one, probably variables reusal where new ones should be used after https://github.com/linuxboot/heads/compare/ffdb947542e263bad9d0da2c673db84543bb4ddf..050e5b8d17be5d6d7439ac949ecb0f6d7ae892fc |
Actually, my bad completely; I had previously written over gbe region with null content over past upgrades manually. So borrowed my v560tu gbe region, injected it in v540tu full backup rom, flashed back and generated intel mac twice and worked flawlessly. But this testing makes me wonder if we could speed things up just reading --ifd -i gbe, writing directy into it with nvm and flashing it back without ifdtool altogether. Will try. Still want ifdtool though for next iterations. |
Phew, I thought here comes the next problem.
That sounds interesting. I'll be away for the next two days, but I'll check it out after that. |
Really interesting gains since most of 32Mib chips is skipped, reading --ifd -i fb and -i gbe is all that is needed for iftool + nvm actions. Problem with this path alone is that Heads complains not being able to succeed upon preserve_rom which is called unconditionally in code as of this pr for now: Still investigating. Also restating that ifdtool will be really helpful in keeping MRC cache on dev cycles and to not scare users; a reminder that current flash.sh doesn't preserve it and results in black screen of around a minute each time a firmware update occurs(since there is no injection of current MRC cache into new rom to be flashed: causing it), this is reinforcing priority of #2138 (since next release will only contain code to deal with it, pushing fix to next flashing to preserve MRC cache across firmware upgrades). |
|
Cannot push directly to The same commits are pushed to tlaurion:mac_randomisation (my personal fork). If a Nitrokey maintainer can cherry-pick, or if someone with push access can run: git fetch tlaurion mac_randomisation
git push Nitrokey tlaurion/mac_randomisation:mac_randomisationthe branch is ready to fast-forward into the PR. Two commits, ahead of
Both signed-off by Thierry Laurion. Verified on v540tu (novacustom-v540tu / novacustom-v560tu): the gbe_only path no longer errors on |
Follow-up to the PoC in #1195. This PR builds on that work and it is a feature to view and randomize the Ethernet MAC address stored in the GbE region of the flash.
Module
Makefile
initrd/bin/gui-init.sh
initrd/etc/functions.sh
initrd/bin/flash.sh
board/
GUI
New submenu under the main menu, shown only when both CONFIG_NVMUTIL and CONFIG_IFDTOOL are set to yes for the board:
Tested boards