[PATCH] Makefile: perform sanity checks on payload during build

[Samuel Holland]

Hi Tim,

On 2024-10-04 8:14 AM, Tim Hutt wrote:
> To make mistakes more obvious and debugging easier we can check that the payload is not an ELF file, and that the first byte is possibly the start of a valid instruction.
> 
> I would have preferred to not do this check in Bash but I didn't want to introduce any additional dependencies, and there isn't a proper language already in use.
> 
> I also made it an `#error` to not define `FW_PAYLOAD_PATH` since the build system will always do that. If the user doesn't specify one a default file is used.

You need a Signed-off-by. Please also wrap the commit message to 72 characters.

> ---
>  firmware/external_deps.mk |  3 ++-
>  firmware/fw_payload.S     |  6 ++----
>  scripts/check-payload.sh  | 43 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 5 deletions(-)
>  create mode 100755 scripts/check-payload.sh
> 
> diff --git a/firmware/external_deps.mk b/firmware/external_deps.mk
> index 6264005..d4c480c 100644
> --- a/firmware/external_deps.mk
> +++ b/firmware/external_deps.mk
> @@ -10,5 +10,6 @@
>  $(platform_build_dir)/firmware/fw_dynamic.o: $(FW_FDT_PATH)
>  $(platform_build_dir)/firmware/fw_jump.o: $(FW_FDT_PATH)
>  $(platform_build_dir)/firmware/fw_payload.o: $(FW_FDT_PATH)
> -
> +	scripts/check-payload.sh $(FW_PAYLOAD_PATH_FINAL)
> +	$(call compile_as,$@,$<)

I would suggest leaving the recipe for fw_payload.o alone, and using a separate,
possibly phony, target for this check. It's surprising to have the recipe for
assembling this one file defined in a separate file.

>  $(platform_build_dir)/firmware/fw_payload.o: $(FW_PAYLOAD_PATH_FINAL)
> diff --git a/firmware/fw_payload.S b/firmware/fw_payload.S
> index 3c8433e..0b0f342 100644
> --- a/firmware/fw_payload.S
> +++ b/firmware/fw_payload.S
> @@ -94,8 +94,6 @@ fw_options:
>  	.globl payload_bin
>  payload_bin:
>  #ifndef FW_PAYLOAD_PATH
> -	wfi
> -	j	payload_bin
> -#else
> -	.incbin	FW_PAYLOAD_PATH
> +#error FW_PAYLOAD_PATH should always be set by the build system
>  #endif
> +	.incbin	FW_PAYLOAD_PATH
> diff --git a/scripts/check-payload.sh b/scripts/check-payload.sh
> new file mode 100755
> index 0000000..5752497
> --- /dev/null
> +++ b/scripts/check-payload.sh
> @@ -0,0 +1,43 @@
> +#!/usr/bin/env bash

With the below changes this script can be POSIX-compatible and use /bin/sh.

> +
> +# Simple script to try and detect invalid payloads. The payload should be a flat
> +# image and the first bytes should be valid instructions - OpenSBI will jump
> +# to the start of the file. It shouldn't be an ELF file.
> +#
> +# Bash is a terrible language for this purpose but we can make it work.
> +
> +if [ -z "$1" ]; then
> +    echo "Usage: $0 <file>"
> +    exit 1
> +fi
> +
> +# Check the payload is not an ELF file.
> +readelf -h "$1" >/dev/null 2>&1 && {

This should get $READELF from Makefile; see how that file sets OBJCOPY.

> +    echo "Error: The payload is an ELF file; it should be a flat executable instead."
> +    exit 1
> +}
> +
> +# Check the first bytes are a valid RISC-V instruction. Since we don't know
> +# what is valid we can only do limited checks but this will check that it is at
> +# least a 16 or 32-bit instruction. Larger instructions are currently unused.
> +first_byte="$(head -c 1 "$1")"
> +LC_ALL=C printf -v first_byte_dec %d \'$first_byte

This can be done portably with od(1):

first_byte=$(od -An -N1 -td1 "$1")

> +
> +# for byte in range(256):
> +#     if not ((byte & 0b11) != 0b11 or (byte & 0b11100) != 0b11100):
> +#         print(byte)
> +[ \
> +    "$first_byte_dec" == 31 -o \
> +    "$first_byte_dec" == 63 -o \
> +    "$first_byte_dec" == 95 -o \
> +    "$first_byte_dec" == 127 -o \
> +    "$first_byte_dec" == 159 -o \
> +    "$first_byte_dec" == 191 -o \
> +    "$first_byte_dec" == 223 -o \
> +    "$first_byte_dec" == 255 \

You can use arithmetic expansion here (which is also POSIX):

[ $((first_byte & 31)) -eq 31 ]

> +] && {
> +    echo "Error: The payload's first byte is not a 16- or 32-bit RISC-V instruction. The payload should be a flat executable."
> +    exit 1
> +}
> +
> +exit 0

No need for an explicit "exit 0" at the end.

Regards,
Samuel