Skip to content

Support symcrypt fips config for aboot/uboot #10729

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.

Sign up for GitHub

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

Merged
xumia merged 4 commits into from
Jun 2, 2022
Merged

Support symcrypt fips config for aboot/uboot #10729

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3367dbd
Support symcrypt fips config for aboot/uboot
xumia May 7, 2022
004371e
Fix image not found issue
xumia May 7, 2022
3b89cf7
Remove the fips options
xumia May 18, 2022
004d19c
Use the same bash comparing way
xumia May 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions build_image.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ elif [ "$IMAGE_TYPE" = "aboot" ]; then
generate_device_list ".platforms_asic"
zip -g $OUTPUT_ABOOT_IMAGE .platforms_asic

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
Copy link
Copy Markdown
Collaborator

@Staphylo Staphylo May 20, 2022

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.

Copy link
Copy Markdown
Collaborator Author

@xumia xumia May 20, 2022

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.

Copy link
Copy Markdown
Collaborator

@Staphylo Staphylo May 23, 2022

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

Copy link
Copy Markdown
Collaborator Author

@xumia xumia May 23, 2022

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.


zip -g $OUTPUT_ABOOT_IMAGE $ABOOT_BOOT_IMAGE
rm $ABOOT_BOOT_IMAGE
if [ "$SONIC_ENABLE_IMAGE_SIGNATURE" = "y" ]; then
Expand Down
2 changes: 1 addition & 1 deletion files/Aboot/boot0.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

# for backward compatibility with the sonic_upgrade= behavior
install="${install:-${sonic_upgrade:-}}"
Expand Down
3 changes: 3 additions & 0 deletions installer/arm64/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ if [ "$install_env" = "onie" ]; then
fi
fi

extra_cmdline_linux=%%EXTRA_CMDLINE_LINUX%%
echo "EXTRA_CMDLINE_LINUX=$extra_cmdline_linux"

# Update Bootloader Menu with installed image
bootloader_menu_config

Expand Down
3 changes: 3 additions & 0 deletions installer/armhf/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,9 @@ if [ "$install_env" = "onie" ]; then
fi
fi

extra_cmdline_linux=%%EXTRA_CMDLINE_LINUX%%
echo "EXTRA_CMDLINE_LINUX=$extra_cmdline_linux"

# Update Bootloader Menu with installed image
bootloader_menu_config

Expand Down
6 changes: 4 additions & 2 deletions platform/centec-arm64/platform.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ mount_partition() {

bootloader_menu_config() {
if [ "$install_env" = "onie" ]; then
fw_setenv -f linuxargs "${extra_cmdline_linux}"
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"
Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linuxargs

Use extra_cmdline_linux directly? Like your changes in other platform.conf files.

Copy link
Copy Markdown
Collaborator Author

@xumia xumia May 7, 2022 •
edited
Loading

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.

Copy link
Copy Markdown
Collaborator

@qiluo-msft qiluo-msft May 7, 2022

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"

Copy link
Copy Markdown
Collaborator Author

@xumia xumia May 7, 2022 •
edited
Loading

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

fw_setenv -f sonic_image_2 "NONE"
fw_setenv -f sonic_dir_1 $image_dir
fw_setenv -f sonic_dir_2 "NONE"
Expand All @@ -37,9 +38,10 @@ bootloader_menu_config() {
fi
done

fw_setenv linuxargs "${extra_cmdline_linux}"
fw_setenv 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 sonic_image_$idx "ext4load mmc 0:1 \$loadaddr \$sonic_dir_$idx/boot/sonic_arm64.fit && setenv bootargs quiet console=\$consoledev,\$baudrate root=/dev/mmcblk0p1 rw rootwait rootfstype=ext4 loopfstype=squashfs loop=\$sonic_dir_$idx/fs.squashfs systemd.unified_cgroup_hierarchy=0 && bootm \$loadaddr"
fw_setenv sonic_image_$idx "ext4load mmc 0:1 \$loadaddr \$sonic_dir_$idx/boot/sonic_arm64.fit && setenv bootargs quiet console=\$consoledev,\$baudrate root=/dev/mmcblk0p1 rw rootwait rootfstype=ext4 loopfstype=squashfs loop=\$sonic_dir_$idx/fs.squashfs systemd.unified_cgroup_hierarchy=0 \${linuxargs} && bootm \$loadaddr"
fw_setenv sonic_dir_$idx $image_dir
fw_setenv sonic_version_$idx `echo $image_dir | sed "s/^image-/SONiC-OS-/g"`

Expand Down
2 changes: 1 addition & 1 deletion platform/marvell-arm64/platform.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ prepare_boot_menu() {
BORDER='echo "---------------------------------------------------";echo;'
fw_setenv ${FW_ARG} print_menu $BORDER $BOOT1 $BOOT2 $BOOT3 $BORDER > /dev/null

fw_setenv ${FW_ARG} linuxargs "net.ifnames=0 loopfstype=squashfs loop=$image_dir/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG" > /dev/null
fw_setenv ${FW_ARG} linuxargs "net.ifnames=0 loopfstype=squashfs loop=$image_dir/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG ${extra_cmdline_linux}" > /dev/null
fw_setenv ${FW_ARG} linuxargs_old "net.ifnames=0 loopfstype=squashfs loop=$image_dir_old/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG" > /dev/null
sonic_bootargs_old='setenv bootargs root='$demo_dev' rw rootwait rootfstype=ext4 panic=1 console=ttyS0,115200 ${othbootargs} ${mtdparts} ${linuxargs_old}'
fw_setenv ${FW_ARG} sonic_bootargs_old $sonic_bootargs_old > /dev/null || true
Expand Down
2 changes: 1 addition & 1 deletion platform/marvell-armhf/platform.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ prepare_boot_menu() {
BORDER='echo "---------------------------------------------------";echo;'
fw_setenv ${FW_ARG} print_menu $BORDER $BOOT1 $BOOT2 $BOOT3 $BORDER > /dev/null

fw_setenv ${FW_ARG} linuxargs "net.ifnames=0 loopfstype=squashfs loop=$image_dir/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG loglevel=4" > /dev/null
fw_setenv ${FW_ARG} linuxargs "net.ifnames=0 loopfstype=squashfs loop=$image_dir/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG loglevel=4 ${extra_cmdline_linux}" > /dev/null
fw_setenv ${FW_ARG} linuxargs_old "net.ifnames=0 loopfstype=squashfs loop=$image_dir_old/$FILESYSTEM_SQUASHFS systemd.unified_cgroup_hierarchy=0 varlog_size=$VAR_LOG loglevel=4" > /dev/null

# Set boot configs
Expand Down