-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Support symcrypt fips config for aboot/uboot #10729
Conversation
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
files/Aboot/boot0.j2
Outdated
@@ -86,7 +86,7 @@ installer_image_path="$image_path/$installer_image" | |||
|
|||
boot_config="$target_path/boot-config" | |||
|
|||
cmdline_allowlist="crashkernel hwaddr_ma1" | |||
cmdline_allowlist="crashkernel hwaddr_ma1 sonic_fips fips" |
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.
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 will add in the Design doc, the option fips is taken by linux kernel option, we are not ready for it now, so use sonic_fips instead.
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.
files/Aboot/boot0.j2
Outdated
@@ -634,6 +634,11 @@ write_image_specific_cmdline() { | |||
# systemd.show_status=false or quiet can be used to silence systemd entierly | |||
cmdline_add systemd.show_status=auto | |||
|
|||
# fips configuration | |||
{%- if ENABLE_FIPS == "y" %} | |||
cmdline_add sonic_fips=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.
fw_setenv -f nos_bootcmd "test -n \$boot_once && setenv do_boot_once \$boot_once && setenv boot_once && saveenv && run do_boot_once; run boot_next" | ||
|
||
fw_setenv -f sonic_image_1 "ext4load mmc 0:1 \$loadaddr \$sonic_dir_1/boot/sonic_arm64.fit && setenv bootargs quiet console=\$consoledev,\$baudrate root=/dev/mmcblk0p1 rw rootwait rootfstype=ext4 loopfstype=squashfs loop=\$sonic_dir_1/fs.squashfs systemd.unified_cgroup_hierarchy=0 && bootm \$loadaddr" | ||
fw_setenv -f sonic_image_1 "ext4load mmc 0:1 \$loadaddr \$sonic_dir_1/boot/sonic_arm64.fit && setenv bootargs quiet console=\$consoledev,\$baudrate root=/dev/mmcblk0p1 rw rootwait rootfstype=ext4 loopfstype=squashfs loop=\$sonic_dir_1/fs.squashfs systemd.unified_cgroup_hierarchy=0 \${linuxargs} && bootm \$loadaddr" |
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.
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.
Currently, marvell arm64/armhf has already used the parameter linuxargs, we do not want to define another parameter for centec-arm64.
And it will be easy to add a Cli for FIPS setting for uboot, without considering different platforms.
Only the uboot boodloader uses it, the other platforms do not use it.
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 am proposing
fw_setenv -f sonic_image_1 "ext4load mmc 0:1 \$loadaddr \$sonic_dir_1/boot/sonic_arm64.fit && setenv bootargs quiet console=\$consoledev,\$baudrate root=/dev/mmcblk0p1 rw rootwait rootfstype=ext4 loopfstype=squashfs loop=\$sonic_dir_1/fs.squashfs systemd.unified_cgroup_hierarchy=0 \${extra_cmdline_linux} && bootm \$loadaddr"
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.
It makes the centec uboot config the same as marvell when adding a command line support, see uboot.py in https://github.com/Azure/sonic-utilities/pull/2154/files
@Staphylo Could you check Aboot change? |
ea69fcd
to
ec22e67
Compare
@Staphylo , could you please help review it? Thanks. |
f75f0ce
to
3b89cf7
Compare
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.
Sorry for the late review, I had a wrong filter. Please feel free to shoot me an email directly if this happens again.
build_image.sh
Outdated
echo "sonic_fips=0" > kernel-cmdline | ||
[ "$ENABLE_FIPS" == "y" ] && echo "sonic_fips=1" > kernel-cmdline | ||
zip -g $OUTPUT_ABOOT_IMAGE kernel-cmdline | ||
rm kernel-cmdline |
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.
Your solution using kernel-cmdline
will work.
One note, even though the kernel handles multiple definitions of the same parameter properly, it's probably not worth crowding the cmdline when enabling fips (will append sonic_fips=0 sonic_fips=1
which can be confusing)
If the plan is to configure FIPS only at build time I would suggest making a modification to boot0.j2
and add a new variable enable_fips
set via Jinja templating to true
or false
.
Then adding some logic in the boot0
code to call cmdline_add
in the write_image_specific_config
function
https://github.com/Azure/sonic-buildimage/blob/f6927606b3720d4f526b9e734b4431f012d21ee3/files/Aboot/boot0.j2#L617
This would prevent anyone from disabling sonic_fips
by changing the cmdline in the context of secureboot.
However if the plan is to be able to disable fips at runtime or for a next reboot, your solution works better.
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.
@Staphylo , it makes the user have chance to enable or disable it in the next reboot. ENABLE_FIPS=1 will enable the fips by default, the default value is "n", not enabled. If want to enable it, we need to set the sonic_fips=1 in kernel-cmdline after installed or upgraded.
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.
Fair enough, your solution is probably the best then.
I still think you should change the code that crafts the kernel-cmdline
content.
I'm thinking of something like the following (Note that the test equality sign is =
and not ==
as you currently have, so there's a bug here)
if [ "$ENABLE_FIPS" = "y" ]; then
echo sonic_fips=1 > kernel-cmdline
else
echo sonic_fips=0 > kernel-cmdline
fi
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.
@Staphylo , thanks for your comment.
The shell bash supports "==", but it makes sense to use the same way with the others in the script, I have changed it.
@Staphylo, could you please help approve it, if you do not have more comments, thanks. |
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
Why I did it Support symcrypt fips config for aboot/uboot
Why I did it
Support symcrypt fips config for aboot/uboot
How I did it
For uboot, use fw_setenv to variable linuxargs to change the boot options.
For Aboot, add the config in /host/image-{version}/kernel-cmdline, example:
How to verify it
Download the image sonic-aboot-broadcom.swi from the pipeline in the PR checks. The sonic_fips=1 can be found in /proc/cmdline
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)