Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6538014:
Add transitional flag for enabling arm kernel signing (Closed)
|
Created:
9 years, 10 months ago by Che-Liang Chiou Modified:
9 years ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionAdd transitional flag for enabling arm kernel signing
For now arm kernel partitions are not signed. This CL is a transitionsl.
That is, the added flag should be removed after arm verified boot is stable.
To properly create an arm kernel partition, we also need another CL for
vbutil_kernel utility that turns off x86-only modifications on kernel
image. See CL:6538015.
BUG=chromium-os:3790, chromium-os:12352
TEST=see below
Build images for x86 and arm successfully, and notice that load_kernel_test
passes for x86 and signed arm image.
$ build_image --board=tegra2_seaboard --crosbug12352_arm_kernel_signing
$ build_image --board=tegra2_seaboard --nocrosbug12352_arm_kernel_signing
$ build_image --board=x86-generic
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=75ac2be
Patch Set 1 #
Total comments: 12
Patch Set 2 : Code review #
Total comments: 8
Patch Set 3 : Code review #
Total comments: 2
Patch Set 4 : Code review #
Messages
Total messages: 11 (0 generated)
Thanks for starting down this treacherous path! :) That said, I'm not a huge fan of this change, but it may be because I don't fully understand why we can't cut over. Is the problem that not everyone working on arm boards will be able to use a default-signed kernel or is it that booting to a signed kernel is still unstable? If this transition is really needed, the argument needs to very specifically call out the problem and we need a crosbug opened to remove it once signing is stable. To be fair, x86 gets optional signing by supporting the legacy boot path (via the EFI Sys Partition) simultaneously with a signed kernel. Is it possible to do the same on the arm boards. E.g., if you're using a non-cros uboot you get legacy behavior, but if you're using our uboot build, you will use the signed kernel? That may be unreasonable, but I'd prefer it to adding complexity to the conditionals. Thanks! http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable File bin/cros_make_image_bootable (right): http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable#ne... bin/cros_make_image_bootable:121: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ same comment from other files :) http://codereview.chromium.org/6538014/diff/1/build_image File build_image (right): http://codereview.chromium.org/6538014/diff/1/build_image#newcode96 build_image:96: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ This flag is way too broad. It is meant as a transitionary crutch for arm, but it wouldn't be obvious to the script user. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode73 build_kernel_image.sh:73: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ This flag, at best, should be called something like --crosbug111222333_temporary_hack_for_arm :) http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode153 build_kernel_image.sh:153: ${FLAGS_enable_kernel_signing} -eq ${FLAGS_TRUE} ]]; then This is making things pretty complex. Why can't we just enable this all normally, then let the behavior be controlled with --enable_rootfs_verification like it is for x86. That was never meant to be an x86 specific flag. If not, can we at least not nest the different behaviors for each platform? E.g., if you want to disable kernel signing, that should standalone, shred code should follow, then it should check for x86/arm specific tweaks. Instead we end up with extra x86 conditionals and two separate areas for the arm boot path. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode192 build_kernel_image.sh:192: notx86="--notx86" ? Why isn't this an --arch arm flag? http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode221 build_kernel_image.sh:221: ${notx86} If this just used the arch, then you wouldn't need extra autodetection.
Like Will, I am missing the background on why this change is required. Can you describe why this is needed? On Thu, Feb 17, 2011 at 8:54 AM, <wad@chromium.org> wrote: > Thanks for starting down this treacherous path! :) > > > That said, I'm not a huge fan of this change, but it may be because I don't > fully understand why we can't cut over. > > Is the problem that not everyone working on arm boards will be able to use a > default-signed kernel or is it that booting to a signed kernel is still > unstable? > > If this transition is really needed, the argument needs to very specifically > call out the problem and we need a crosbug opened to remove it once signing > is > stable. > > To be fair, x86 gets optional signing by supporting the legacy boot path > (via > the EFI Sys Partition) simultaneously with a signed kernel. Is it possible > to > do the same on the arm boards. E.g., if you're using a non-cros uboot you > get > legacy behavior, but if you're using our uboot build, you will use the > signed > kernel? > > That may be unreasonable, but I'd prefer it to adding complexity to the > conditionals. > > Thanks! > > > http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable > File bin/cros_make_image_bootable (right): > > http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable#ne... > bin/cros_make_image_bootable:121: DEFINE_boolean enable_kernel_signing > ${FLAGS_FALSE} \ > same comment from other files :) > > http://codereview.chromium.org/6538014/diff/1/build_image > File build_image (right): > > http://codereview.chromium.org/6538014/diff/1/build_image#newcode96 > build_image:96: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ > This flag is way too broad. It is meant as a transitionary crutch for > arm, but it wouldn't be obvious to the script user. > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh > File build_kernel_image.sh (right): > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode73 > build_kernel_image.sh:73: DEFINE_boolean enable_kernel_signing > ${FLAGS_FALSE} \ > This flag, at best, should be called something like > > --crosbug111222333_temporary_hack_for_arm :) > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode153 > build_kernel_image.sh:153: ${FLAGS_enable_kernel_signing} -eq > ${FLAGS_TRUE} ]]; then > This is making things pretty complex. > > Why can't we just enable this all normally, then let the behavior be > controlled with --enable_rootfs_verification like it is for x86. That > was never meant to be an x86 specific flag. > > If not, can we at least not nest the different behaviors for each > platform? E.g., if you want to disable kernel signing, that should > standalone, shred code should follow, then it should check for x86/arm > specific tweaks. Instead we end up with extra x86 conditionals and two > separate areas for the arm boot path. > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode192 > build_kernel_image.sh:192: notx86="--notx86" > ? Why isn't this an --arch arm flag? > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode221 > build_kernel_image.sh:221: ${notx86} > If this just used the arch, then you wouldn't need extra autodetection. > > http://codereview.chromium.org/6538014/ > -- -g
Thanks for the consideration. Comments below. On 2011/02/17 16:54:09, Will Drewry wrote: > Thanks for starting down this treacherous path! :) > > > That said, I'm not a huge fan of this change, but it may be because I don't > fully understand why we can't cut over. > > Is the problem that not everyone working on arm boards will be able to use a > default-signed kernel or is it that booting to a signed kernel is still > unstable? It is the latter; booting to a signed ARM kernel is still unstable. The purpose of this patch is to: 1. Let the rest of firmware team be able to develop and test verified boot, and 2. Not block other part of development, like kernel, if verified boot is unstable or fail to boot; no one should sync to ToT and then find out that it couldn't boot. So I decide to limit the arm signing process to when you explicitly say it. > > If this transition is really needed, the argument needs to very specifically > call out the problem and we need a crosbug opened to remove it once signing is > stable. This transition is unnecessary if no one else is using/testing verified boot -- I don't think this is the case. Alternatively, this patch is not pushed, and users/testers of verified boot may apply this patch themselves. This sounds to me like more repetitive work for others. > > To be fair, x86 gets optional signing by supporting the legacy boot path (via > the EFI Sys Partition) simultaneously with a signed kernel. Is it possible to > do the same on the arm boards. E.g., if you're using a non-cros uboot you get > legacy behavior, but if you're using our uboot build, you will use the signed > kernel? First of all, I don't think it is a legacy because, as far as I know, there is no standard booting protocol to find, load, and boot a kernel on arm. In x86 we have EFI and etc., but in arm we don't have any equivalent (arm linux kernel has a documented interface for passing parameters and etc., but it doesn't specify how you find and load kernel image to RAM). IMO, how we boot an unsigned kernel is another work-around rather than a legacy. To be qualified as a legacy, x86-generic chromeos image is bootable on any PC. But I don't think it makes sense to make arm chromeos image that universal. In addition, the "legacy" stores u-boot script at the beginning of kernel partition, which conflicts with key block of a signed kernel. So we need more work-arounds to stop a non-cros u-boot loading script here in a signed image. One way or another, we can't avoid changing image building process to inject more u-boot scripts into EFI partition so that we can override bottom-half of non-cros u-boot's booting process. > > That may be unreasonable, but I'd prefer it to adding complexity to the > conditionals. > > Thanks! > > http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable > File bin/cros_make_image_bootable (right): > > http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable#ne... > bin/cros_make_image_bootable:121: DEFINE_boolean enable_kernel_signing > ${FLAGS_FALSE} \ > same comment from other files :) > > http://codereview.chromium.org/6538014/diff/1/build_image > File build_image (right): > > http://codereview.chromium.org/6538014/diff/1/build_image#newcode96 > build_image:96: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ > This flag is way too broad. It is meant as a transitionary crutch for arm, but > it wouldn't be obvious to the script user. > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh > File build_kernel_image.sh (right): > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode73 > build_kernel_image.sh:73: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ > This flag, at best, should be called something like > > --crosbug111222333_temporary_hack_for_arm :) > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode153 > build_kernel_image.sh:153: ${FLAGS_enable_kernel_signing} -eq ${FLAGS_TRUE} ]]; > then > This is making things pretty complex. > > Why can't we just enable this all normally, then let the behavior be controlled > with --enable_rootfs_verification like it is for x86. That was never meant to > be an x86 specific flag. > > If not, can we at least not nest the different behaviors for each platform? > E.g., if you want to disable kernel signing, that should standalone, shred code > should follow, then it should check for x86/arm specific tweaks. Instead we end > up with extra x86 conditionals and two separate areas for the arm boot path. > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode192 > build_kernel_image.sh:192: notx86="--notx86" > ? Why isn't this an --arch arm flag? > > http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode221 > build_kernel_image.sh:221: ${notx86} > If this just used the arch, then you wouldn't need extra autodetection.
Hi, I made changes based on your comments. Please take another look. Thanks very much. Regards, Che-Liang http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable File bin/cros_make_image_bootable (right): http://codereview.chromium.org/6538014/diff/1/bin/cros_make_image_bootable#ne... bin/cros_make_image_bootable:121: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ On 2011/02/17 16:54:09, Will Drewry wrote: > same comment from other files :) Done. http://codereview.chromium.org/6538014/diff/1/build_image File build_image (right): http://codereview.chromium.org/6538014/diff/1/build_image#newcode96 build_image:96: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ On 2011/02/17 16:54:09, Will Drewry wrote: > This flag is way too broad. It is meant as a transitionary crutch for arm, but > it wouldn't be obvious to the script user. Done. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode73 build_kernel_image.sh:73: DEFINE_boolean enable_kernel_signing ${FLAGS_FALSE} \ On 2011/02/17 16:54:09, Will Drewry wrote: > This flag, at best, should be called something like > > --crosbug111222333_temporary_hack_for_arm :) Done. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode153 build_kernel_image.sh:153: ${FLAGS_enable_kernel_signing} -eq ${FLAGS_TRUE} ]]; then On 2011/02/17 16:54:09, Will Drewry wrote: > This is making things pretty complex. > > Why can't we just enable this all normally, then let the behavior be controlled > with --enable_rootfs_verification like it is for x86. That was never meant to > be an x86 specific flag. > > If not, can we at least not nest the different behaviors for each platform? > E.g., if you want to disable kernel signing, that should standalone, shred code > should follow, then it should check for x86/arm specific tweaks. Instead we end > up with extra x86 conditionals and two separate areas for the arm boot path. I rewrote this part of logic. I hope I did not misunderstand your suggestion. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode192 build_kernel_image.sh:192: notx86="--notx86" On 2011/02/17 16:54:09, Will Drewry wrote: > ? Why isn't this an --arch arm flag? Because it was meant to turn-off x86-only operation. I will rename it to --arch. http://codereview.chromium.org/6538014/diff/1/build_kernel_image.sh#newcode221 build_kernel_image.sh:221: ${notx86} On 2011/02/17 16:54:09, Will Drewry wrote: > If this just used the arch, then you wouldn't need extra autodetection. Done.
LGTM with optional changes This looks good, but it could be tweaked a bit more to keep all the related logic together. If you want to, you can remerge it one more time and let me know and I'll take a look, or you can go ahead with it as is assuming you've retested the TEST= options with the new version :) Thanks! http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:189: error "Unknown arch: ${FLAGS_arch}" This isn't quite what I meant. I was hoping to de-dupe even more and only have one conditional for x86 and one for arm. However, this helps a lot. One suggestion below. http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:195: "${FLAGS_crosbug12352_arm_kernel_signing}" -eq "${FLAGS_TRUE}" ]]; then Instead of doing this test here, would it be easier to just add a variable like: sign_the_kernel=${FLAGS_FALSE} then in the x86 section above set: sign_the_kernel=${FLAGS_TRUE} and in the arm section do: sign_the_kernel=${FLAGS_crosbug12352_arm_kernel_signing} and here just check: if [ $sign_the_kernel -eq ${FLAGS_TRUE} ]; then ... fi If that change was made, you could also then move the arm-unsigned code up under the arm conditional above instead of leaving it stranded off the signing else.
lgtm http://codereview.chromium.org/6538014/diff/5001/build_image File build_image (right): http://codereview.chromium.org/6538014/diff/5001/build_image#newcode813 build_image:813: [[ "${ARCH}" = "arm" && \ you don't need the '\' here http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:181: tr '\n' ' ' <"${FLAGS_working_dir}/boot.config" >> "${bootloader_script}" space between < and "
Hi Will, I made changes based on your comments. And also reformat the code a little bit. Please take another look. Thanks very much. http://codereview.chromium.org/6538014/diff/5001/build_image File build_image (right): http://codereview.chromium.org/6538014/diff/5001/build_image#newcode813 build_image:813: [[ "${ARCH}" = "arm" && \ On 2011/02/22 22:07:02, gauravsh wrote: > you don't need the '\' here Done. http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:181: tr '\n' ' ' <"${FLAGS_working_dir}/boot.config" >> "${bootloader_script}" On 2011/02/22 22:07:02, gauravsh wrote: > space between < and " Done. http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:189: error "Unknown arch: ${FLAGS_arch}" On 2011/02/22 19:33:31, Will Drewry wrote: > This isn't quite what I meant. I was hoping to de-dupe even more and only have > one conditional for x86 and one for arm. However, this helps a lot. One > suggestion below. Done. http://codereview.chromium.org/6538014/diff/5001/build_kernel_image.sh#newcod... build_kernel_image.sh:195: "${FLAGS_crosbug12352_arm_kernel_signing}" -eq "${FLAGS_TRUE}" ]]; then On 2011/02/22 19:33:31, Will Drewry wrote: > Instead of doing this test here, would it be easier to just add a variable like: > sign_the_kernel=${FLAGS_FALSE} > > then in the x86 section above set: > sign_the_kernel=${FLAGS_TRUE} > and in the arm section do: > sign_the_kernel=${FLAGS_crosbug12352_arm_kernel_signing} > > > and here just check: > > if [ $sign_the_kernel -eq ${FLAGS_TRUE} ]; then > ... > fi > > > If that change was made, you could also then move the arm-unsigned code up under > the arm conditional above instead of leaving it stranded off the signing else. Done.
LGTM after fixing the one nit. Thanks for streamlining it as much as possible given the messiness of the task. Might check with gauravsh too just to make sure it seems sane to him. cheers! will http://codereview.chromium.org/6538014/diff/10001/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/10001/build_kernel_image.sh#newco... build_kernel_image.sh:150: mkdir -p ${FLAGS_working_dir} Wasn't this called on line 126?
lgtm
Thanks for the suggestions. They made this CL much clearer. http://codereview.chromium.org/6538014/diff/10001/build_kernel_image.sh File build_kernel_image.sh (right): http://codereview.chromium.org/6538014/diff/10001/build_kernel_image.sh#newco... build_kernel_image.sh:150: mkdir -p ${FLAGS_working_dir} On 2011/02/23 18:46:07, Will Drewry wrote: > Wasn't this called on line 126? Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
