Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6253014:
Add script to validate kernel params before we sign images (Closed)
|
Created:
9 years, 11 months ago by jimhebert Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Randall Spangler, Luigi Semenzato, Bill Richardson, gauravsh Visibility:
Public. |
DescriptionAdd script to validate kernel params before we sign images
Change-Id: I8ffedf8afa00862d135f80db9350927cc0332979
BUG=chrome-os-partner:1991
TEST=Have run it manually with various config data producing test-pass and the different sources of test-fails
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=d7c9e82
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixes for review feedback. #
Total comments: 5
Patch Set 3 : Fixes for nits #
Messages
Total messages: 5 (0 generated)
Thanks in advance for your review!
http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... File scripts/image_signing/ensure_secure_kernelparams.config (right): http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.config:21: required_dmparams_common="vroot none ro,0 1740800 verity /dev/sd%D%P \ comment about where do you get this parameter from? I think the 1750800 number is based on the rootfs size - a mention of that would be nice. http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... File scripts/image_signing/ensure_secure_kernelparams.sh (right): http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:3: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. 2011 http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:14: echo "Usage $0 <image>" nit: use $PROG instead of $0 (for consistency, nothing else) http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:19: . "$(dirname "$0")/${0/%.sh/.config}" the substitution is a bit cryptic. I would suggest explicitly naming the expectations file here. Better yet, make the expectation file be an argument (maybe, optional) to the script. http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:45: testpass=true all globals should be all uppercase. if you prefer, you can encapsulate the whole loop into a main() function and a "main $@" that starts off the main script. http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:53: board=$(grep CHROMEOS_RELEASE_BOARD= "$rootfs/etc/lsb-release" | \ Note that this will not work on an already signed image since we change the release board then. (x86-mario may become x86-mario-pvtkeys etc.) http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:60: kparams="$(dump_kernel_config "$kernelblob")" nit: i think you probably don't need the extra quotes around $(). There is an unofficial chrome os sh style guide which talks about avoid excessive quoting. Can't seem to find it now. http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:73: for param in ${required_kparams[@]}; do : probably not a very big deal - but how will this handle changes in the order of individual params, or repetitions. I am not sure is there are kernel options like that - where order matters. http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... scripts/image_signing/ensure_secure_kernelparams.sh:96: if $testpass; then instead of testpass, you could just use testfail=0 in the beginning, and set it to 1 when a failure occurs. then here, simply exit $testfail
Thanks! PTAL. Since the diff is messy (due to the main() mass-indent), I have included notes in-line below. On Wed, Jan 26, 2011 at 2:55 PM, <gauravsh@chromium.org> wrote: > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > File scripts/image_signing/ensure_secure_kernelparams.config (right): > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.config:21: > required_dmparams_common="vroot none ro,0 1740800 verity /dev/sd%D%P \ > comment about where do you get this parameter from? I think the 1750800 > number is based on the rootfs size - a mention of that would be nice. > Done. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > File scripts/image_signing/ensure_secure_kernelparams.sh (right): > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:3: # Copyright (c) > 2010 The Chromium OS Authors. All rights reserved. > 2011 > Done. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:14: echo "Usage $0 > <image>" > nit: use $PROG instead of $0 (for consistency, nothing else) > Done. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:19: . "$(dirname > "$0")/${0/%.sh/.config}" > the substitution is a bit cryptic. I would suggest explicitly naming the > expectations file here. Better yet, make the expectation file be an > argument (maybe, optional) to the script. > Optional command-line-argument added. Commenting added as well to help address the cryptic-ness of what it does in the default case. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:45: testpass=true > all globals should be all uppercase. if you prefer, you can encapsulate > the whole loop into a main() function and a "main $@" that starts off > the main script. > I opted for main() > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:53: board=$(grep > CHROMEOS_RELEASE_BOARD= "$rootfs/etc/lsb-release" | \ > Note that this will not work on an already signed image since we change > the release board then. (x86-mario may become x86-mario-pvtkeys etc.) > Fixed. It meant that cut | tr got replaced with sed but I added a comment to mitigate any cryptic-ness that brought on. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:60: > kparams="$(dump_kernel_config "$kernelblob")" > nit: i think you probably don't need the extra quotes around $(). There > is an unofficial chrome os sh style guide which talks about avoid > excessive quoting. Can't seem to find it now. > Good feedback, I cleaned up several cases of this throughout the script. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:73: for param in > ${required_kparams[@]}; do : > probably not a very big deal - but how will this handle changes in the > order of individual params, or repetitions. I am not sure is there are > kernel options like that - where order matters. > Yeah I had the same thought and the same conclusion that I don't think it will come up. The closest thing I can see (order matters inside the dm="..." values) is covered by it being special cased. > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > scripts/image_signing/ensure_secure_kernelparams.sh:96: if $testpass; > then > instead of testpass, you could just use testfail=0 in the beginning, and > set it to 1 when a failure occurs. > > then here, simply exit $testfail Done - thanks! > > > http://codereview.chromium.org/6253014/ >
LGTM with some style nits. Please fix and test before submitting. http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... File scripts/image_signing/ensure_secure_kernelparams.config (right): http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_secure_kernelparams.config:21: # taken from observation of current builds. In particular we may see nit: sentence seems to read wrong. maybe something like "we may see the size of the filesystem creep over in time... http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... File scripts/image_signing/ensure_secure_kernelparams.sh (right): http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_secure_kernelparams.sh:57: testfail=0 define this at the beginning of the function (and add a comment). Also, for vars that have function scope, it is a good idea to mark them clearly as such by defining them as local. http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_secure_kernelparams.sh:67: sed 's/.*=\([^-]*\)-\([^-]*\).*/\1_\2/') cut -f 1-2 -d '-' | sed -e s/-/_/ maybe a tad bit easier to to read than the sed command. your call. :) http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_secure_kernelparams.sh:85: # Ensure all other required params are present nit: general comment about comments - typically all comment sentences must end in a period. http://codereview.chromium.org/6253014/diff/6001/scripts/image_signing/ensure... scripts/image_signing/ensure_secure_kernelparams.sh:87: if [[ "$kparams_nodm" != *$param* ]]; then nit: use one of either [ ] or [[ ]]. recommended is to just use [ ]
Also, before submitting, edit the issue description to reference the bug correctly. It should be BUG=chrome-os-partner:1991 On 2011/01/27 21:38:22, jimhebert wrote: > Thanks! PTAL. Since the diff is messy (due to the main() mass-indent), I > have included notes in-line below. > > On Wed, Jan 26, 2011 at 2:55 PM, <mailto:gauravsh@chromium.org> wrote: > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > File scripts/image_signing/ensure_secure_kernelparams.config (right): > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.config:21: > > required_dmparams_common="vroot none ro,0 1740800 verity /dev/sd%D%P \ > > comment about where do you get this parameter from? I think the 1750800 > > number is based on the rootfs size - a mention of that would be nice. > > > > Done. > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > File scripts/image_signing/ensure_secure_kernelparams.sh (right): > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:3: # Copyright (c) > > 2010 The Chromium OS Authors. All rights reserved. > > 2011 > > > > Done. > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:14: echo "Usage $0 > > <image>" > > nit: use $PROG instead of $0 (for consistency, nothing else) > > > > Done. > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:19: . "$(dirname > > "$0")/${0/%.sh/.config}" > > the substitution is a bit cryptic. I would suggest explicitly naming the > > expectations file here. Better yet, make the expectation file be an > > argument (maybe, optional) to the script. > > > > Optional command-line-argument added. Commenting added as well to help > address the cryptic-ness of what it does in the default case. > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:45: testpass=true > > all globals should be all uppercase. if you prefer, you can encapsulate > > the whole loop into a main() function and a "main $@" that starts off > > the main script. > > > > I opted for main() > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:53: board=$(grep > > CHROMEOS_RELEASE_BOARD= "$rootfs/etc/lsb-release" | \ > > Note that this will not work on an already signed image since we change > > the release board then. (x86-mario may become x86-mario-pvtkeys etc.) > > > > Fixed. It meant that cut | tr got replaced with sed but I added a comment to > mitigate any cryptic-ness that brought on. > > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:60: > > kparams="$(dump_kernel_config "$kernelblob")" > > nit: i think you probably don't need the extra quotes around $(). There > > is an unofficial chrome os sh style guide which talks about avoid > > excessive quoting. Can't seem to find it now. > > > > Good feedback, I cleaned up several cases of this throughout the script. > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:73: for param in > > ${required_kparams[@]}; do : > > probably not a very big deal - but how will this handle changes in the > > order of individual params, or repetitions. I am not sure is there are > > kernel options like that - where order matters. > > > > Yeah I had the same thought and the same conclusion that I don't think it > will come up. > > The closest thing I can see (order matters inside the dm="..." values) is > covered by it being special cased. > > > > > > > > > > http://codereview.chromium.org/6253014/diff/1/scripts/image_signing/ensure_se... > > scripts/image_signing/ensure_secure_kernelparams.sh:96: if $testpass; > > then > > instead of testpass, you could just use testfail=0 in the beginning, and > > set it to 1 when a failure occurs. > > > > then here, simply exit $testfail > > > Done - thanks! > > > > > > > > http://codereview.chromium.org/6253014/ > > |
|||||||||||||||||||||||||||||||||||||||||||||||
