From 1fa4b708ba9e98ea776d49c2c93cff7903f4d747 Mon Sep 17 00:00:00 2001 From: Greeva Patel <128170613+Greeva48@users.noreply.github.com> Date: Wed, 28 Oct 2026 15:33:45 +0530 Subject: [PATCH 1/2] fix(imgpack): reject firmware images bootloader cannot load --- tests/unit/test_imgpack.py | 56 +++++++++++++++++++++++++++-- tools/imgpack.py | 74 ++++++++++++++++++++++++++++++++++---- 2 files changed, 120 insertions(+), 10 deletions(-) diff --git a/tests/unit/test_imgpack.py b/tests/unit/test_imgpack.py index 7cee682..00f8e88 100644 --- a/tests/unit/test_imgpack.py +++ b/tests/unit/test_imgpack.py @@ -25,13 +25,20 @@ IMAGE_VERSION_OFFSET = 20 -def _pack(tmp_path, version): - (tmp_path / "fw.bin").write_bytes(b"\x5A" * 256) +def _pack( + tmp_path, + version, + *, + load_addr="0x08010000", + entry_addr="0x08010010", + firmware=b"\x5A" * 256, +): + (tmp_path / "fw.bin").write_bytes(firmware) return subprocess.run( [sys.executable, str(TOOLS / "imgpack.py"), "--input", str(tmp_path / "fw.bin"), "--output", str(tmp_path / "fw.eimg"), - "--load-addr", "0x08010000", "--entry-addr", "0x08010100", + "--load-addr", load_addr, "--entry-addr", entry_addr, # `--version=X`, not `--version X`: a value such as "-1.0.0" is a # separate token in the second form, and whether argparse reads it # as a value or as an unknown option depends on the interpreter -- @@ -72,3 +79,46 @@ def test_out_of_range_version_is_rejected(tmp_path, version): assert not (tmp_path / "fw.eimg").exists(), ( f"output image was written for rejected version '{version}'") assert "out of range" in r.stderr + + +@pytest.mark.parametrize( + ("load_addr", "entry_addr"), + [ + ("0x08010000", "0x08010100"), # exclusive image end + ("0x08010000", "0x0800FFFF"), # before the image + ("0x08010000", "0x100000000"), # entry does not fit uint32 + ("0x100000000", "0x100000000"), # load does not fit uint32 + ("0xFFFFFFF0", "0xFFFFFFF0"), # load + 256-byte image wraps + ], +) +def test_invalid_runtime_layout_is_rejected_before_output( + tmp_path, load_addr, entry_addr +): + r = _pack( + tmp_path, "1.2.3", load_addr=load_addr, entry_addr=entry_addr + ) + assert r.returncode != 0 + assert not (tmp_path / "fw.eimg").exists() + assert "Error:" in r.stderr + + +def test_empty_firmware_is_rejected(tmp_path): + r = _pack(tmp_path, "1.2.3", firmware=b"") + assert r.returncode != 0 + assert not (tmp_path / "fw.eimg").exists() + assert "image size" in r.stderr + + +def test_cli_rejects_input_equal_to_output(tmp_path): + same_path = tmp_path / "firmware.bin" + same_path.write_bytes(b"\x5A" * 256) + r = subprocess.run( + [sys.executable, str(TOOLS / "imgpack.py"), + "--input", str(same_path), "--output", str(same_path), + "--version=1.2.3", "--load-addr", "0x08010000", + "--entry-addr", "0x08010010"], + capture_output=True, text=True, + ) + assert r.returncode != 0 + assert same_path.read_bytes() == b"\x5A" * 256 + assert "must be different" in r.stderr diff --git a/tools/imgpack.py b/tools/imgpack.py index 03bcf01..33ad152 100644 --- a/tools/imgpack.py +++ b/tools/imgpack.py @@ -24,6 +24,8 @@ EOS_IMAGE_HDR_VERSION = 1 EOS_HASH_SIZE = 32 EOS_SIG_MAX_SIZE = 64 +UINT32_MAX = 0xFFFFFFFF +MAX_IMAGE_SIZE = 16 * 1024 * 1024 # Signature types SIG_NONE = 0 @@ -56,6 +58,39 @@ def compute_crc32(data: bytes) -> int: return zlib.crc32(data) & 0xFFFFFFFF +def parse_uint32(value: str, label: str) -> int: + """Parse a hexadecimal CLI value that fits the image's uint32 fields.""" + try: + parsed = int(value, 16) + except ValueError as exc: + raise ValueError(f"{label} must be a hexadecimal uint32, got '{value}'") from exc + if not 0 <= parsed <= UINT32_MAX: + raise ValueError(f"{label} is outside the uint32 range: '{value}'") + return parsed + + +def validate_image_layout(image_size: int, load_addr: int, entry_addr: int) -> None: + """Reject images the bootloader will refuse before writing an artifact.""" + if image_size <= 0 or image_size > MAX_IMAGE_SIZE: + raise ValueError( + f"image size must be between 1 and {MAX_IMAGE_SIZE} bytes; got {image_size}" + ) + if not 0 <= load_addr <= UINT32_MAX or not 0 <= entry_addr <= UINT32_MAX: + raise ValueError("load and entry addresses must fit uint32") + if load_addr == 0: + # The firmware parser treats zero as an unset load address and skips + # the runtime entry-range check for compatibility with XIP targets. + return + if image_size > UINT32_MAX - load_addr: + raise ValueError("load address plus image size overflows uint32") + image_end = load_addr + image_size + if entry_addr < load_addr or entry_addr >= image_end: + raise ValueError( + f"entry address 0x{entry_addr:08X} is outside the image range " + f"[0x{load_addr:08X}, 0x{image_end:08X})" + ) + + def build_header( image_size: int, load_addr: int, @@ -66,6 +101,12 @@ def build_header( ) -> bytes: """Build the eos_image_header_t binary structure.""" + validate_image_layout(image_size, load_addr, entry_addr) + if not 0 <= image_version <= UINT32_MAX: + raise ValueError("image version must fit uint32") + if not 0 <= image_crc <= UINT32_MAX or not 0 <= flags <= UINT32_MAX: + raise ValueError("image CRC and flags must fit uint32") + # Hash field: store CRC32 in first 4 bytes, rest zeroed hash_field = struct.pack('> 24) & 0xFF ver_minor = (image_version >> 16) & 0xFF From 41314a0e7f544cdb36371487017dff267029ba50 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Fri, 30 Oct 2026 10:09:42 +0530 Subject: [PATCH 2/2] fix(imgpack): write the image through an exclusive temporary file The previous commit wrote every image to the fixed name .tmp before renaming it. That name was never checked, so: - `--input fw.eimg.tmp --output fw.eimg` truncated the input firmware and then moved it away; - a symlink planted at .tmp was followed and its target overwritten; - two packs to the same output shared one temporary file. The rename also replaced an output symlink instead of writing through it, and installed the image with default permissions instead of the existing output's. write_output_atomically() now creates the temporary file with tempfile.mkstemp in the destination directory: exclusive, uniquely named, never a pre-existing path or symlink. It writes and fsyncs through that descriptor, then os.replace()s the destination. If the output is a symlink, its target is replaced and the link is kept. An existing output's mode is copied; a new output gets 0666 & ~umask, as a plain create would. The temporary file is removed on any failure, and a failed removal is reported instead of swallowed. The input size is checked with stat() before the file is read, and the read is capped at the limit plus one byte, so an oversized input cannot exhaust memory. Tests: exactly 16 MiB accepted, one byte more rejected, load address 0 skips the entry range check (as core/image_verify.c does), an input at .tmp stays intact, a symlink there is not followed, permissions are preserved on repack and match a plain create on a new output, and a symlinked output is written through. The out-of-range load-address row now uses a valid entry address and each layout row asserts its own reason. Docs: the input limits are in the imgpack.py docstring, docs/quickstart.md and docs/installation_usage.md (whose imgpack command is corrected to the real options), mirrored in llms-full.txt. --- docs/installation_usage.md | 6 +- docs/quickstart.md | 8 ++- llms-full.txt | 14 ++++- tests/unit/test_imgpack.py | 125 ++++++++++++++++++++++++++++++++++--- tools/imgpack.py | 83 +++++++++++++++++++++--- 5 files changed, 213 insertions(+), 23 deletions(-) diff --git a/docs/installation_usage.md b/docs/installation_usage.md index cc6b006..eafef96 100644 --- a/docs/installation_usage.md +++ b/docs/installation_usage.md @@ -158,8 +158,10 @@ void main(void) { # Install Python tool dependencies python3 -m pip install -r requirements.txt -# Pack firmware with eboot image header -python tools/imgpack.py firmware.bin --output firmware.packed.bin +# Pack firmware with eboot image header. The input limits (size, load and +# entry addresses) are listed in quickstart.md and in tools/imgpack.py. +python tools/imgpack.py --input firmware.bin --output firmware.packed.bin \ + --version 1.0.0 --load-addr 0x08010000 --entry-addr 0x08010100 # Sign the image python tools/sign_image.py firmware.packed.bin --key signing_key.pem diff --git a/docs/quickstart.md b/docs/quickstart.md index 638b448..19e1eaf 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -94,7 +94,13 @@ python3 sign_image.py --genkey --output keys/ cmake -B build -DEBLDR_BOARD= -DCMAKE_BUILD_TYPE=Release \ -DEBLDR_PRODUCTION_KEY=$(cat keys/public_key.hex) -# Pack a raw firmware binary into an .eimg container. +# Pack a raw firmware binary into an .eimg container. imgpack refuses what +# the bootloader refuses (core/image_verify.c): an input that is empty or +# over 16 MiB (16777216 bytes), checked before it is read; addresses or +# flags that do not fit 32 bits; and, when --load-addr is non-zero, a load +# address + size past 0xFFFFFFFF or an --entry-addr outside +# [load-addr, load-addr + size). --load-addr 0 means "unset" and skips the +# entry check, as the bootloader does. python3 imgpack.py --input firmware.bin --output firmware.eimg \ --load-addr 0x08010000 --entry-addr 0x08010100 --version 1.0.0 diff --git a/llms-full.txt b/llms-full.txt index cf6dc61..ddd4f9d 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -1695,8 +1695,10 @@ void main(void) { # Install Python tool dependencies python3 -m pip install -r requirements.txt -# Pack firmware with eboot image header -python tools/imgpack.py firmware.bin --output firmware.packed.bin +# Pack firmware with eboot image header. The input limits (size, load and +# entry addresses) are listed in quickstart.md and in tools/imgpack.py. +python tools/imgpack.py --input firmware.bin --output firmware.packed.bin \ + --version 1.0.0 --load-addr 0x08010000 --entry-addr 0x08010100 # Sign the image python tools/sign_image.py firmware.packed.bin --key signing_key.pem @@ -1864,7 +1866,13 @@ python3 sign_image.py --genkey --output keys/ cmake -B build -DEBLDR_BOARD= -DCMAKE_BUILD_TYPE=Release \ -DEBLDR_PRODUCTION_KEY=$(cat keys/public_key.hex) -# Pack a raw firmware binary into an .eimg container. +# Pack a raw firmware binary into an .eimg container. imgpack refuses what +# the bootloader refuses (core/image_verify.c): an input that is empty or +# over 16 MiB (16777216 bytes), checked before it is read; addresses or +# flags that do not fit 32 bits; and, when --load-addr is non-zero, a load +# address + size past 0xFFFFFFFF or an --entry-addr outside +# [load-addr, load-addr + size). --load-addr 0 means "unset" and skips the +# entry check, as the bootloader does. python3 imgpack.py --input firmware.bin --output firmware.eimg \ --load-addr 0x08010000 --entry-addr 0x08010100 --version 1.0.0 diff --git a/tests/unit/test_imgpack.py b/tests/unit/test_imgpack.py index 00f8e88..2ad98e2 100644 --- a/tests/unit/test_imgpack.py +++ b/tests/unit/test_imgpack.py @@ -12,6 +12,8 @@ identically to '1.0.0' — numerically *older* than 1.0.1). """ +import os +import stat import struct import subprocess import sys @@ -22,7 +24,12 @@ REPO_ROOT = Path(__file__).resolve().parents[2] TOOLS = REPO_ROOT / "tools" +IMAGE_SIZE_OFFSET = 8 IMAGE_VERSION_OFFSET = 20 +MAX_IMAGE_SIZE = 16 * 1024 * 1024 # core/image_verify.c + +posix_only = pytest.mark.skipif( + os.name != "posix", reason="needs POSIX symlinks and permission bits") def _pack( @@ -49,6 +56,21 @@ def _pack( capture_output=True, text=True) +def _pack_paths(input_path, output_path): + return subprocess.run( + [sys.executable, str(TOOLS / "imgpack.py"), + "--input", str(input_path), "--output", str(output_path), + "--version=1.2.3", "--load-addr", "0x08010000", + "--entry-addr", "0x08010010"], + capture_output=True, text=True, + ) + + +def _leftover_temporaries(directory): + return sorted(p.name for p in directory.iterdir() + if p.name.startswith(".") and p.name.endswith(".tmp")) + + def test_in_range_version_encodes_correctly(tmp_path): r = _pack(tmp_path, "1.2.3") assert r.returncode == 0, r.stderr @@ -82,17 +104,19 @@ def test_out_of_range_version_is_rejected(tmp_path, version): @pytest.mark.parametrize( - ("load_addr", "entry_addr"), + ("load_addr", "entry_addr", "reason"), [ - ("0x08010000", "0x08010100"), # exclusive image end - ("0x08010000", "0x0800FFFF"), # before the image - ("0x08010000", "0x100000000"), # entry does not fit uint32 - ("0x100000000", "0x100000000"), # load does not fit uint32 - ("0xFFFFFFF0", "0xFFFFFFF0"), # load + 256-byte image wraps + ("0x08010000", "0x08010100", "outside the image range"), # exclusive image end + ("0x08010000", "0x0800FFFF", "outside the image range"), # before the image + ("0x08010000", "0x100000000", "entry address"), # entry does not fit uint32 + # Only the load address is wrong here, so the rejection can come from + # nothing else. + ("0x100000000", "0x08010010", "load address"), # load does not fit uint32 + ("0xFFFFFFF0", "0xFFFFFFF0", "overflows uint32"), # load + 256-byte image wraps ], ) def test_invalid_runtime_layout_is_rejected_before_output( - tmp_path, load_addr, entry_addr + tmp_path, load_addr, entry_addr, reason ): r = _pack( tmp_path, "1.2.3", load_addr=load_addr, entry_addr=entry_addr @@ -100,6 +124,7 @@ def test_invalid_runtime_layout_is_rejected_before_output( assert r.returncode != 0 assert not (tmp_path / "fw.eimg").exists() assert "Error:" in r.stderr + assert reason in r.stderr def test_empty_firmware_is_rejected(tmp_path): @@ -109,6 +134,92 @@ def test_empty_firmware_is_rejected(tmp_path): assert "image size" in r.stderr +def test_image_of_exactly_16_mib_is_accepted(tmp_path): + r = _pack(tmp_path, "1.2.3", firmware=b"\x5A" * MAX_IMAGE_SIZE) + assert r.returncode == 0, r.stderr + data = (tmp_path / "fw.eimg").read_bytes() + assert struct.unpack_from(" int: return parsed -def validate_image_layout(image_size: int, load_addr: int, entry_addr: int) -> None: - """Reject images the bootloader will refuse before writing an artifact.""" +def validate_image_size(image_size: int) -> None: + """Reject an image size the bootloader refuses (0, or over 16 MiB).""" if image_size <= 0 or image_size > MAX_IMAGE_SIZE: raise ValueError( f"image size must be between 1 and {MAX_IMAGE_SIZE} bytes; got {image_size}" ) + + +def validate_image_layout(image_size: int, load_addr: int, entry_addr: int) -> None: + """Reject images the bootloader will refuse before writing an artifact.""" + validate_image_size(image_size) if not 0 <= load_addr <= UINT32_MAX or not 0 <= entry_addr <= UINT32_MAX: raise ValueError("load and entry addresses must fit uint32") if load_addr == 0: @@ -131,6 +155,44 @@ def build_header( return header +def write_output_atomically(output_path: Path, data: bytes) -> None: + """Write data to output_path so that a failed run leaves no partial image. + + The data goes to a temporary file that mkstemp creates exclusively, with + a unique name, in the destination directory: it can never be the input, + another run's file or a planted symlink. It is then renamed over the + destination. An output symlink is written through, as a direct write + would: its target is replaced and the link is left in place. + """ + target = output_path.resolve() if output_path.is_symlink() else output_path + fd, temp_name = tempfile.mkstemp( + dir=target.parent, prefix=f".{target.name}.", suffix=".tmp") + try: + with os.fdopen(fd, "wb") as temp_file: + temp_file.write(data) + temp_file.flush() + os.fsync(temp_file.fileno()) + try: + # Repacking must not loosen an existing image's permissions. + shutil.copymode(target, temp_name) + except FileNotFoundError: + # A new output gets what a plain create would give it; mkstemp's + # 0600 would be stricter than the old direct write. + umask = os.umask(0) + os.umask(umask) + os.chmod(temp_name, 0o666 & ~umask) + os.replace(temp_name, target) + except BaseException: + try: + os.unlink(temp_name) + except FileNotFoundError: + pass + except OSError as cleanup_error: + print(f"Warning: cannot remove temporary file {temp_name}: " + f"{cleanup_error}", file=sys.stderr) + raise + + def main(): parser = argparse.ArgumentParser( description='eBootloader Image Packer — creates .eimg images' @@ -155,10 +217,17 @@ def main(): sys.exit(1) try: - firmware_data = input_path.read_bytes() + # Check the size before reading, and never read more than one byte + # past the limit, so an oversized input cannot exhaust memory. + validate_image_size(input_path.stat().st_size) + with input_path.open("rb") as input_file: + firmware_data = input_file.read(MAX_IMAGE_SIZE + 1) except OSError as e: print(f"Error: cannot read input file: {e}", file=sys.stderr) sys.exit(1) + except ValueError as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1) image_size = len(firmware_data) try: @@ -181,15 +250,9 @@ def main(): flags=flags, ) - temporary_path = output_path.with_name(output_path.name + ".tmp") try: - temporary_path.write_bytes(header + firmware_data) - temporary_path.replace(output_path) + write_output_atomically(output_path, header + firmware_data) except OSError as e: - try: - temporary_path.unlink(missing_ok=True) - except OSError: - pass print(f"Error: cannot write output image: {e}", file=sys.stderr) sys.exit(1)