[cbootimage PATCH v1] Enhance sign script

[cbootimage PATCH v1] Enhance sign script

[Jimmy Zhang]

1. Add parameter <soc> to sign tegra124 bootimage. Previously it can only sign
   for tegra210
2. Automatically generated signed bct. A signed bct is needed during flashing.

Jimmy Zhang (1):
  Add more features

 samples/sign.sh | 115 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 81 insertions(+), 34 deletions(-)

-- 
1.9.1

[cbootimage PATCH v1] Add more features

[Jimmy Zhang]

1. use parameter <soc> to specify boot image type. ie, tegra124, tegra210
2. Along signing bootimage, also generate signed bct, ie, tegra124.bct,
   tegra210.bct. User should use this signed bct when flashing target.

Example:

   $ ./sign.sh tegra124 t124.img rsa_priv.pem

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 samples/sign.sh | 115 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 81 insertions(+), 34 deletions(-)

diff --git a/samples/sign.sh b/samples/sign.sh
index 2edd12695f4b..776d3cbd5ff7 100755
--- a/samples/sign.sh
+++ b/samples/sign.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 #
-# Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
+# Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
 #
 # This program is free software; you can redistribute it and/or modify it
 # under the terms and conditions of the GNU General Public License,
@@ -18,11 +18,8 @@
 # project.
 #
 set -e
-IMAGE_FILE=$1
-KEY_FILE=$2
-TARGET_IMAGE=$IMAGE_FILE
-CONFIG_FILE=config.tmp
 
+CONFIG_FILE=config.tmp
 CBOOTIMAGE=../src/cbootimage
 BCT_DUMP=../src/bct_dump
 OBJCOPY=objcopy
@@ -33,41 +30,91 @@ MV=mv
 XXD=xxd
 CUT=cut
 
-echo "Get rid of all temporary files: *.sig, *.tosig, *.tmp *.mod"
-$RM -f *.sig *.tosig *.tmp *.mod
 
-echo "Get bl length "
-BL_LENGTH=`$BCT_DUMP $IMAGE_FILE | grep "Bootloader\[0\].Length"\
- | awk -F ' ' '{print $4}' | awk -F ';' '{print $1}'`
+usage ()
+{
+	echo -e "
+Usage: ./sign.sh <soc> <boot_image> <rsa_priv_key>
+  Where,
+	soc: tegra124, tegra210
+	boot_image: image generated by cbootimage,
+	priv_key: rsa key file in .pem format."
+
+	exit 1;
+}
+
+sign_image ()
+{
+	local bct_length=$(($3 + $4));
+
+	echo "Get bl length "
+	local bl_length=`$BCT_DUMP $IMAGE_FILE | grep "Bootloader\[0\].Length"\
+	 | awk -F ' ' '{print $4}' | awk -F ';' '{print $1}'`
+
+	echo "Extract bootloader to $IMAGE_FILE.bl.tosig, length ${bl_length}"
+	$DD bs=1 skip=$2 if=$IMAGE_FILE of=$IMAGE_FILE.bl.tosig \
+	 count=${bl_length}
+
+	echo "Calculate rsa signature for bootloader and save to $IMAGE_FILE.bl.sig"
+	$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
+		 -sign $KEY_FILE -out $IMAGE_FILE.bl.sig $IMAGE_FILE.bl.tosig
 
-echo "Extract bootloader to $IMAGE_FILE.bl.tosig, length $BL_LENGTH"
-$DD bs=1 skip=32768 if=$IMAGE_FILE of=$IMAGE_FILE.bl.tosig count=$BL_LENGTH
+	echo "Update bootloader's rsa signature, aes hash and bct's aes hash"
+	echo "RsaPssSigBlFile = $IMAGE_FILE.bl.sig;" > $CONFIG_FILE
+	echo "RehashBl;" >> $CONFIG_FILE
+	$CBOOTIMAGE -s $1 -u $CONFIG_FILE $IMAGE_FILE $IMAGE_FILE.tmp
 
-echo "Calculate rsa signature for bootloader and save to $IMAGE_FILE.bl.sig"
-$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
- -sign $KEY_FILE -out $IMAGE_FILE.bl.sig $IMAGE_FILE.bl.tosig
+	echo "Extract the part of bct which needs to be rsa signed"
+	$DD bs=1 if=$IMAGE_FILE.tmp of=$IMAGE_FILE.bct.tosig skip=$3 count=$4
 
-echo "Update bootloader's rsa signature, aes hash and bct's aes hash"
-echo "RsaPssSigBlFile = $IMAGE_FILE.bl.sig;" > $CONFIG_FILE
-echo "RehashBl;" >> $CONFIG_FILE
-$CBOOTIMAGE -s tegra210 -u $CONFIG_FILE $IMAGE_FILE $IMAGE_FILE.tmp
+	echo "Calculate rsa signature for bct and save to $IMAGE_FILE.bct.sig"
+	$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
+		 -sign $KEY_FILE -out $IMAGE_FILE.bct.sig $IMAGE_FILE.bct.tosig
 
-echo "Extract the part of bct which needs to be rsa signed"
-$DD bs=1 if=$IMAGE_FILE.tmp of=$IMAGE_FILE.bct.tosig count=8944 skip=1296
+	echo "Create public key modulus from key file $KEY_FILE and save to $KEY_FILE.mod"
+	$OPENSSL rsa -in $KEY_FILE -noout -modulus -out $KEY_FILE.mod
+	# remove prefix
+	$CUT -d= -f2 < $KEY_FILE.mod > $KEY_FILE.mod.tmp
 
-echo "Calculate rsa signature for bct and save to $IMAGE_FILE.bct.sig"
-$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
- -sign $KEY_FILE -out $IMAGE_FILE.bct.sig $IMAGE_FILE.bct.tosig
+	# convert from hexdecimal to binary
+	$XXD -r -p -l 256 $KEY_FILE.mod.tmp $KEY_FILE.mod.bin
 
-echo "Create public key modulus from key file $KEY_FILE and save to $KEY_FILE.mod"
-$OPENSSL rsa -in $KEY_FILE -noout -modulus -out $KEY_FILE.mod
-# remove prefix
-$CUT -d= -f2 < $KEY_FILE.mod > $KEY_FILE.mod.tmp
+	echo "Update bct's rsa signature and modulus"
+	echo "RsaPssSigBctFile = $IMAGE_FILE.bct.sig;" > $CONFIG_FILE
+	echo "RsaKeyModulusFile = $KEY_FILE.mod.bin;" >> $CONFIG_FILE
+	echo ""
+	$CBOOTIMAGE -s $1 -u $CONFIG_FILE $IMAGE_FILE.tmp $TARGET_IMAGE
+
+	echo ""
+	$DD bs=1 if=$TARGET_IMAGE of=${soc}.bct count=${bct_length}
+	echo ""
+	echo "Signed bct ${soc}.bct has been successfully generated!";
+
+	#echo "Get rid of all temporary files: *.sig, *.tosig, *.tmp, *.mod, *.mod.bin"
+	$RM -f *.sig *.tosig *.tmp *.mod *.mod.bin
+}
+
+
+soc=$1		# tegra124, tegra210
+
+if [[ "${soc}" == tegra124 ]]; then
+	bl_block_offset=16384;  # emmc: 16384, spi_flash: 32768: default: emmc
+	bct_signed_offset=1712;
+	bct_signed_length=6480;
+elif [ "${soc}" = tegra210 ]; then
+	bl_block_offset=32768;  # emmc: 16384, spi_flash: 32768: default: spi
+	bct_signed_offset=1296;
+	bct_signed_length=8944;
+elif [[ "${soc}" != tegra124 && \
+        "${soc}" != tegra210 ]]; then
+        echo "Error: Invalid target device($soc).";
+        usage;
+fi;
+
+IMAGE_FILE=$2;
+KEY_FILE=$3;
+TARGET_IMAGE=$IMAGE_FILE
 
-# convert from hexdecimal to binary
-$XXD -r -p -l 256 $KEY_FILE.mod.tmp $KEY_FILE.mod.bin
+echo "Sign ${soc} ${IMAGE_FILE} with key ${KEY_FILE}"
+sign_image "$soc" "$bl_block_offset" "$bct_signed_offset" "$bct_signed_length"
 
-echo "Update bct's rsa signature and modulus"
-echo "RsaPssSigBctFile = $IMAGE_FILE.bct.sig;" > $CONFIG_FILE
-echo "RsaKeyModulusFile = $KEY_FILE.mod.bin;" >> $CONFIG_FILE
-$CBOOTIMAGE -s tegra210 -u $CONFIG_FILE $IMAGE_FILE.tmp $TARGET_IMAGE
-- 
1.9.1

Re: [cbootimage PATCH v1] Add more features

[Stephen Warren]

On 04/08/2016 08:11 PM, Jimmy Zhang wrote:
> 1. use parameter <soc> to specify boot image type. ie, tegra124, tegra210
> 2. Along signing bootimage, also generate signed bct, ie, tegra124.bct,
>     tegra210.bct. User should use this signed bct when flashing target.
>
> Example:
>
>     $ ./sign.sh tegra124 t124.img rsa_priv.pem

> diff --git a/samples/sign.sh b/samples/sign.sh

> -# Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
> +# Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.

Copyright years should be added to the list, not replace the list. See 
our internal copyright wiki. Put another way, "2015-2016" not "2016".

> +usage ()
> +{
> +	echo -e "
> +Usage: ./sign.sh <soc> <boot_image> <rsa_priv_key>
> +  Where,
> +	soc: tegra124, tegra210
> +	boot_image: image generated by cbootimage,
> +	priv_key: rsa key file in .pem format."
> +
> +	exit 1;
> +}

Interesting; I don't think I've ever seen multi-line text passed to 
echo. I guess if it works, then it's fine. I think the following syntax 
is more common though:

cat <<EOF
Usage: ...
...
EOF

> +sign_image ()
> +{
> +	local bct_length=$(($3 + $4));

Rather than using $1..$4, can you do this:

something=$1
other=$2
...

and then use ${something} and ${other} etc. throughout. That will make 
the code a bit more readable, as well as document the function signature 
a little.

> +soc=$1		# tegra124, tegra210
> +
> +if [[ "${soc}" == tegra124 ]]; then
> +	bl_block_offset=16384;  # emmc: 16384, spi_flash: 32768: default: emmc
> +	bct_signed_offset=1712;
> +	bct_signed_length=6480;
> +elif [ "${soc}" = tegra210 ]; then
> +	bl_block_offset=32768;  # emmc: 16384, spi_flash: 32768: default: spi
> +	bct_signed_offset=1296;
> +	bct_signed_length=8944;
> +elif [[ "${soc}" != tegra124 && \
> +        "${soc}" != tegra210 ]]; then

You know that's not true given the if/elif conditions. Why not just use 
"else" here?

> +echo "Sign ${soc} ${IMAGE_FILE} with key ${KEY_FILE}"
> +sign_image "$soc" "$bl_block_offset" "$bct_signed_offset" "$bct_signed_length"

This patch would be a bit easier to read if it didn't convert everything 
to a function at the same time, which introduces another indentation 
level and hence makes the entire script into a big diff. Is there a 
reason to convert everything to a function? I could understand this if 
the code were split up into a bunch of functions and they were called 
multiple times, but as far as I can tell that isn't the case here.

If you want to convert everything to a function, I suggest a separate 
patch for that (which is basically just a whitespace change) v.s. adding 
new functionality.

Re: [cbootimage PATCH v1] Enhance sign script

[Stephen Warren]

On 04/08/2016 08:11 PM, Jimmy Zhang wrote:
> 1. Add parameter <soc> to sign tegra124 bootimage. Previously it can only sign
>     for tegra210
> 2. Automatically generated signed bct. A signed bct is needed during flashing.
>
> Jimmy Zhang (1):
>    Add more features

When there is just one patch, or even when there are multiple patches 
but there's no need for a series-level explanation, please don't send a 
cover letter since it adds no information.

When sending a cover letter, please name it "PATCH 0/n", and name the 
patches "m/n". "git format-patch" and "git format-patch --cover-letter" 
will do this automatically, so it's less work to do the right thing:-)