[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt <xypron.debian@gmx.de>]

On 12/11/19 4:10 PM, Cristian Ciocaltea wrote:
> On Wed, Dec 11, 2019 at 10:57:48AM +0100, Heinrich Schuchardt wrote:
>> On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
>>> On Tue, Dec 10, 2019 at 08:32:17PM +0100, Heinrich Schuchardt wrote:
>>>> On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
>>>>> Add support for booting EFI binaries contained in FIT images.
>>>>> A typical usage scenario is chain-loading GRUB2 in a verified
>>>>> boot environment.
>>>>>
>>>>> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>
>>>>
>>>> Reading through the code it looks good. What I really need to do is
>>>> analyze the address usage on the sandbox. To me it is unclear if
>>>> images->fdt_addr is a physical address or an address in the address
>>>> space of the sandbox.
>>>>
>>>> Did you test this on the sandbox? You can use
>>>> lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
>>>> command for loading the FIT image.
>>>
>>> I only tested on qemu, I've never used the sandbox, so it's a good
>>> opportunity to give it a try.
>>>
>>>> Shouldn't we add booting a UEFI FIT image to the Python test in
>>>> test/py/tests/test_fit.py?
>>>
>>> Unfortunately I'm not familiar with the testing framework (including
>>> Python scripting), but I'll do my best to add such a test.
>>>
>>>> doc/uImage.FIT/signature.txt describes that several properties of the
>>>> RSA public key should be stored in the control device tree.
>>>> Unfortunately no example is supplied in which format they should be
>>>> stored. Could you send me an example, please.
>>>>
>>>> I found the following
>>>>
>>>> https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb
>>>>
>>>> Is this an accurate description? Or how do you get the parameters from
>>>> your RSA public key?
>>>
>>> My test scenario involves the following steps:
>>>
>>> 1. Create a public/private key pair
>>> $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
>>>           -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
>>>
>>> 2. Create a certificate containing the public key
>>> $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
>>>
>>> 3. Dump QEMU virt board DTB
>>> $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
>>>           -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
>>>
>>> 4. Create (unsigned) FIT image and put the public key into DTB, with
>>>      the 'required' property set, telling U-Boot that this key MUST be
>>>      verified for the image to be valid
>>> $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
>>>
>>> 5. Sign the FIT image
>>> $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
>>>
>>> 6. Run QEMU supplying the DTB containing the public key and the
>>>      u-boot binary built with CONFIG_OF_BOARD
>>> $ qemu-system-arm -nographic \
>>>       -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
>>>       -dtb ${BOARD_DTB} [...]
>>>
>>> This is what I get after booting QEMU with the command above:
>>>
>>> => fdt addr $fdtcontroladdr
>>> => fdt print
>>> / {
>>>       [...]
>>> 	signature {
>>> 		key-dev {
>>> 			required = "conf";
>>> 			algo = "sha256,rsa2048";
>>> 			rsa,r-squared = * 0x5ef05188 [0x00000100];
>>> 			rsa,modulus = * 0x5ef05294 [0x00000100];
>>> 			rsa,exponent = <0x00000000 0x00010001>;
>>> 			rsa,n0-inverse = <0x649cd557>;
>>> 			rsa,num-bits = <0x00000800>;
>>> 			key-name-hint = "dev";
>>> 		};
>>> 	};
>>>       [...]
>>
>> See my patch
>>
>> doc: fitImage: example of a signature node
>> https://lists.denx.de/pipermail/u-boot/2019-December/393613.html
>>
>> ---
>>
>> Booting of the sandbox fails due to an incorrect address passed for the
>> device tree:
>>
>> => host load hostfs - $kernel_addr_r image.fit
>> 26558 bytes read in 0 ms
>> => bootm ${kernel_addr_r}#config-grub-fdt
>> ## Loading kernel from FIT Image at 01000000 ...
>>     Using 'config-grub-fdt' configuration
>>     Verifying Hash Integrity ... OK
>>     Trying 'helloworld' kernel subimage
>>       Description:  Hello World
>>       Created:      2019-12-11   9:19:22 UTC
>>       Type:         Kernel Image (no loading done)
>>       Compression:  uncompressed
>>       Data Start:   0x010000cc
>>       Data Size:    2733 Bytes = 2.7 KiB
>>       Hash algo:    sha256
>>       Hash value:
>> 5c3ba35a1cb4abfe8a867ea6ac709574535794a7d7d03ba1ec2273b956d13983
>>     Verifying Hash Integrity ... sha256+ OK
>>     XIP Kernel Image (no loading done)
>> ## Loading fdt from FIT Image at 01000000 ...
>>     Using 'config-grub-fdt' configuration
>>     Verifying Hash Integrity ... OK
>>     Trying 'fdt-test' fdt subimage
>>       Description:  QEMU DTB
>>       Created:      2019-12-11   9:19:22 UTC
>>       Type:         Flat Device Tree
>>       Compression:  uncompressed
>>       Data Start:   0x01000c74
>>       Data Size:    19713 Bytes = 19.3 KiB
>>       Architecture: ARM
>>       Hash algo:    sha256
>>       Hash value:
>> 3e4f4e2b512f7a03a7f9ccecfb2b6bf7ceea75882370639460fd61502d903cd1
>>     Verifying Hash Integrity ... sha256+ OK
>>     Booting using the fdt blob at 0x1000c74
>> Found 0 disks
>> phys_to_virt: Cannot map sandbox address 11001c74 (SDRAM from 0 to 8000000)
>> Aborted
>
> I've checked the internal handling of the FDT images and it seems the
> 'ft_addr' attribute inside 'bootm_headers_t' structure points to a
> mapped memory location. Since efi_install_fdt() assumes a physical
> address (it calls map_sysmem() before accessing the data), it might be
> enough to just reconvert 'ft_addr' via map_to_sysmem(), prior the call
> to efi_install_fdt().

I will change my patch "efi_loader: export efi_install_fdt()" to expect
a pointer to addressable memory instead of a "physical" address. This
will avoid double conversions.

>
> The issue is not present on ARM because the memory mapping utilities do
> not actually perform any operations.
>
>> Please, check both the FDT and the non-FDT case on the sandbox and
>> resubmit the patch.
>
> Yes, I'm going to (re)test on both QEMU and sandbox.
>
> Should the upcoming patch set also include the suggested python test
> or that can be added later as a separate patch?

The patches will not be merged into U-Boot master before the v2020.01
release on January 6th. If you send a follow up patch until then, it is
also fine.

Best regards

Heinrich

>
> Thanks.
>
>> Best regards
>>
>> Heinrich
>