[PATCH v1 0/3] fdt_support: validate property lengths in chosen and dma-range fixups

[PATCH v1 0/3] fdt_support: validate property lengths in chosen and dma-range fixups

[Aristo Chen]

boot/fdt_support.c contains a number of helpers that fix up the kernel
devicetree handed to the OS during bootm/booti. Several of those
helpers consume fdt_getprop() results without validating the returned
length against the per-entry size implied by the surrounding cell-count
arithmetic. When the OS devicetree is not signature-verified, for
example an unsigned FIT, a DT loaded from $fdtaddr or $fdtcontroladdr,
or a DT supplied over a network boot, the property is
attacker-influenced and the missing checks turn into out-of-bounds
reads or writes on the FDT blob and on stack buffers.

The first patch targets fdt_fixup_stdout(). The function copies the
value of /aliases/serialN into a fixed 256-byte stack buffer before
publishing it as /chosen/linux,stdout-path, but does not check that
the property fits. The patch rejects an oversized property with a
warning and -FDT_ERR_NOSPACE so the unbounded memcpy cannot run.

The second patch addresses fdt_get_dma_range(). The function reads one
full dma-ranges entry of (na + pna + ns) * sizeof(u32) bytes after
checking only that the returned length is non-zero. A dma-ranges
property shorter than one entry causes the subsequent fdt_read_number()
and fdt_translate_dma_address() calls to read past the property within
the FDT blob. The patch validates the length against one full entry
and returns -EINVAL when the property is too short, matching the
existing failure paths in this function.

The third patch is an unrelated cleanup. A handful of printf call
sites in fdt_fixup_memory_banks, __of_translate_address and
fdt_get_dma_range still use the gcc-specific __FUNCTION__ identifier
while the rest of the file already uses the C99-standard __func__.
The patch converts the remaining occurrences for consistency with the
rest of the file.

Aristo Chen (3):
  fdt_support: bound serialN alias length before copying to stack
  fdt_support: validate dma-ranges length in fdt_get_dma_range
  fdt_support: prefer __func__ over __FUNCTION__

 boot/fdt_support.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] fdt_support: bound serialN alias length before copying to stack

[Aristo Chen]

fdt_fixup_stdout() reads the path stored in /aliases/serialN with
fdt_getprop() and then memcpys it into a fixed 256-byte stack buffer.
The length returned by libfdt is the raw on-disk property size and is
not bounded by any console-path convention, so an oversized property
in a malformed or untrusted devicetree overflows the buffer with
attacker-controlled length and contents. The "/* long enough */"
comment next to tmp[] codifies an unchecked assumption.

Reject lengths that exceed sizeof(tmp) with a warning and return
-FDT_ERR_NOSPACE. The fixup runs during fdt_chosen() on every booted
kernel when CONFIG_OF_STDOUT_VIA_ALIAS is enabled, and when the OS
devicetree is not signature-verified the property is reachable from an
attacker-influenced blob.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 boot/fdt_support.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index 1c215e548db..3e9445603ff 100644
--- a/boot/fdt_support.c
+++ b/boot/fdt_support.c
@@ -160,6 +160,12 @@ static int fdt_fixup_stdout(void *fdt, int chosenoff)
 		goto noalias;
 	}
 
+	if (len > (int)sizeof(tmp)) {
+		printf("WARNING: %s: %s alias path too long (%d bytes)\n",
+		       __func__, sername, len);
+		return -FDT_ERR_NOSPACE;
+	}
+
 	/* fdt_setprop may break "path" so we copy it to tmp buffer */
 	memcpy(tmp, path, len);
 
-- 
2.43.0

[PATCH v1 2/3] fdt_support: validate dma-ranges length in fdt_get_dma_range

[Aristo Chen]

fdt_get_dma_range() fetches the dma-ranges property with fdt_getprop()
and checks only that the length is non-zero before reading one full
entry from it. The entry size depends on na, pna and ns cells returned
by count_cells, which come from the parent buses in the devicetree.
A dma-ranges property shorter than (na + pna + ns) * sizeof(u32) bytes
causes fdt_read_number() and fdt_translate_dma_address() to read past
the end of the property within the FDT blob, an out-of-bounds read of
attacker-influenced data when the OS devicetree is not signature
verified.

Reject the property when its length is smaller than one full entry and
return -EINVAL, matching the existing failure paths in this function.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 boot/fdt_support.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index 3e9445603ff..c4c2a5d02f6 100644
--- a/boot/fdt_support.c
+++ b/boot/fdt_support.c
@@ -1633,6 +1633,13 @@ int fdt_get_dma_range(const void *blob, int node, phys_addr_t *cpu,
 		goto out;
 	}
 
+	if (len < (int)((na + pna + ns) * sizeof(*ranges))) {
+		printf("%s: dma-ranges too short for %s\n", __func__,
+		       fdt_get_name(blob, node, NULL));
+		ret = -EINVAL;
+		goto out;
+	}
+
 	*bus = fdt_read_number(ranges, na);
 	*cpu = fdt_translate_dma_address(blob, node, ranges + na);
 	*size = fdt_read_number(ranges + na + pna, ns);
-- 
2.43.0

[PATCH v1 3/3] fdt_support: prefer __func__ over __FUNCTION__

[Aristo Chen]

A handful of printf call sites in fdt_fixup_memory_banks,
__of_translate_address and fdt_get_dma_range still use the
gcc-specific __FUNCTION__ identifier. checkpatch.pl prefers the
C99-standard __func__ for new code, and the rest of this file already
uses __func__, so convert the remaining occurrences for consistency.

While in __of_translate_address, also re-align the continuation line
of the inner Bad cell count printf so the second argument lines up
with the open parenthesis of its printf instead of being one column
off, silencing the alignment check that scripts/checkpatch.pl reports
on the cleanup hunk. No behavioural change.

Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
 boot/fdt_support.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index c4c2a5d02f6..14a24d6f641 100644
--- a/boot/fdt_support.c
+++ b/boot/fdt_support.c
@@ -551,13 +551,13 @@ int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[], int banks)
 	if (banks > MEMORY_BANKS_MAX) {
 		printf("%s: num banks %d exceeds hardcoded limit %d."
 		       " Recompile with higher MEMORY_BANKS_MAX?\n",
-		       __FUNCTION__, banks, MEMORY_BANKS_MAX);
+		       __func__, banks, MEMORY_BANKS_MAX);
 		return -1;
 	}
 
 	err = fdt_check_header(blob);
 	if (err < 0) {
-		printf("%s: %s\n", __FUNCTION__, fdt_strerror(err));
+		printf("%s: %s\n", __func__, fdt_strerror(err));
 		return err;
 	}
 
@@ -1503,7 +1503,7 @@ static u64 __of_translate_address(const void *blob, int node_offset,
 	/* Cound address cells & copy address locally */
 	bus->count_cells(blob, parent, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
-		printf("%s: Bad cell count for %s\n", __FUNCTION__,
+		printf("%s: Bad cell count for %s\n", __func__,
 		       fdt_get_name(blob, node_offset, NULL));
 		goto bail;
 	}
@@ -1530,8 +1530,8 @@ static u64 __of_translate_address(const void *blob, int node_offset,
 		pbus = of_match_bus(blob, parent);
 		pbus->count_cells(blob, parent, &pna, &pns);
 		if (!OF_CHECK_COUNTS(pna, pns)) {
-			printf("%s: Bad cell count for %s\n", __FUNCTION__,
-				fdt_get_name(blob, node_offset, NULL));
+			printf("%s: Bad cell count for %s\n", __func__,
+			       fdt_get_name(blob, node_offset, NULL));
 			break;
 		}
 
@@ -1618,7 +1618,7 @@ int fdt_get_dma_range(const void *blob, int node, phys_addr_t *cpu,
 	bus_node = of_match_bus(blob, node);
 	bus_node->count_cells(blob, node, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
-		printf("%s: Bad cell count for %s\n", __FUNCTION__,
+		printf("%s: Bad cell count for %s\n", __func__,
 		       fdt_get_name(blob, node, NULL));
 		return -EINVAL;
 		goto out;
@@ -1627,7 +1627,7 @@ int fdt_get_dma_range(const void *blob, int node, phys_addr_t *cpu,
 	bus_node = of_match_bus(blob, parent);
 	bus_node->count_cells(blob, parent, &pna, &pns);
 	if (!OF_CHECK_COUNTS(pna, pns)) {
-		printf("%s: Bad cell count for %s\n", __FUNCTION__,
+		printf("%s: Bad cell count for %s\n", __func__,
 		       fdt_get_name(blob, parent, NULL));
 		return -EINVAL;
 		goto out;
-- 
2.43.0

Re: [PATCH v1 3/3] fdt_support: prefer __func__ over __FUNCTION__

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

On Mon, May 25, 2026 at 01:26:25PM +0000, Aristo Chen wrote:

> A handful of printf call sites in fdt_fixup_memory_banks,
> __of_translate_address and fdt_get_dma_range still use the
> gcc-specific __FUNCTION__ identifier. checkpatch.pl prefers the
> C99-standard __func__ for new code, and the rest of this file already
> uses __func__, so convert the remaining occurrences for consistency.
> 
> While in __of_translate_address, also re-align the continuation line
> of the inner Bad cell count printf so the second argument lines up
> with the open parenthesis of its printf instead of being one column
> off, silencing the alignment check that scripts/checkpatch.pl reports
> on the cleanup hunk. No behavioural change.
> 
> Signed-off-by: Aristo Chen <aristo.chen@canonical.com>

Oh wow, I didn't realize we still had __FUNCTION__ anywhere in the code.
Can you please move this to it's own series and do a global fixup of
__FUNCTION__ -> __func__ ? Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 0/3] fdt_support: validate property lengths in chosen and dma-range fixups

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Mon, May 25, 2026 at 01:26:22PM +0000, Aristo Chen wrote:

> boot/fdt_support.c contains a number of helpers that fix up the kernel
> devicetree handed to the OS during bootm/booti. Several of those
> helpers consume fdt_getprop() results without validating the returned
> length against the per-entry size implied by the surrounding cell-count
> arithmetic. When the OS devicetree is not signature-verified, for
> example an unsigned FIT, a DT loaded from $fdtaddr or $fdtcontroladdr,
> or a DT supplied over a network boot, the property is
> attacker-influenced and the missing checks turn into out-of-bounds
> reads or writes on the FDT blob and on stack buffers.
> 
> The first patch targets fdt_fixup_stdout(). The function copies the
> value of /aliases/serialN into a fixed 256-byte stack buffer before
> publishing it as /chosen/linux,stdout-path, but does not check that
> the property fits. The patch rejects an oversized property with a
> warning and -FDT_ERR_NOSPACE so the unbounded memcpy cannot run.
> 
> The second patch addresses fdt_get_dma_range(). The function reads one
> full dma-ranges entry of (na + pna + ns) * sizeof(u32) bytes after
> checking only that the returned length is non-zero. A dma-ranges
> property shorter than one entry causes the subsequent fdt_read_number()
> and fdt_translate_dma_address() calls to read past the property within
> the FDT blob. The patch validates the length against one full entry
> and returns -EINVAL when the property is too short, matching the
> existing failure paths in this function.
> 
> The third patch is an unrelated cleanup. A handful of printf call
> sites in fdt_fixup_memory_banks, __of_translate_address and
> fdt_get_dma_range still use the gcc-specific __FUNCTION__ identifier
> while the rest of the file already uses the C99-standard __func__.
> The patch converts the remaining occurrences for consistency with the
> rest of the file.
> 
> Aristo Chen (3):
>   fdt_support: bound serialN alias length before copying to stack
>   fdt_support: validate dma-ranges length in fdt_get_dma_range

I'm a little concerned about the potential size growth of adding
warnings in these cases, can you please check how much the growth is and
move them to debug() if it's non-trivial? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]