[PATCH 0/1] armv8: sec_firmware: validate loadables string list

[PATCH 0/1] armv8: sec_firmware: validate loadables string list

[Josh Law]

Hi folks,

sec_firmware_check_copy_loadable() reads the loadables property with
fdt_getprop(), then walks it with strchr(). That works when the FIT is
well formed. If the property is malformed and the last string is missing
its trailing NUL, the walk can go past the property while looking for
the end of the entry.

The fix is to use the libfdt string list helpers for the walk. Missing
loadables still means there is nothing to copy. A malformed loadables
list now fails before any entry is used.

To check the bad case, I put a three byte loadables value with no
trailing NUL at the end of a readable page and ran the old strchr()
loop. It faults when strchr() crosses into the guard page. I also
checked the patched file still builds with:

  make O=/tmp/u-boot-sec-fw-build CROSS_COMPILE=aarch64-linux-gnu- \
       -j$(nproc) arch/arm/cpu/armv8/sec_firmware.o

I did not add the reproducer as a new test file. I couldn't find an
existing sec_firmware test harness, and adding one file for this single
case felt like churn. This is the standalone patch I used to check the
old loop. Save this as testbug.c:

  // SPDX-License-Identifier: GPL-2.0+
  #define _GNU_SOURCE
  #include <string.h>
  #include <sys/mman.h>
  #include <unistd.h>
  
  int main(void)
  {
  	const char *str;
  	long page;
  	char *area;
  	char *name;
  	int len = 3;
  
  	page = sysconf(_SC_PAGESIZE);
  	if (page <= 0)
  		return 1;
  
  	area = mmap(NULL, page * 2, PROT_READ | PROT_WRITE,
  		    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
  	if (area == MAP_FAILED)
  		return 1;
  	if (mprotect(area + page, page, PROT_NONE))
  		return 1;
  
  	name = area + page - len;
  	memcpy(name, "tee", len);
  
  	for (str = name; str && ((str - name) < len);
  	     str = strchr(str, '\0') + 1) {
  	}
  
  	return 0;
  }

Josh Law (1):
  armv8: sec_firmware: validate loadables string list

 arch/arm/cpu/armv8/sec_firmware.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.47.3

[PATCH 1/1] armv8: sec_firmware: validate loadables string list

[Josh Law]

sec_firmware_check_copy_loadable() walks the loadables property by hand
and treats each entry as a C string. If a malformed property is missing
the trailing NUL inside its length, strchr() can read past the property
while looking for the end of the entry.

Use libfdt string list helpers for the walk. Missing loadables still
means there is nothing to copy, and malformed loadables now fail before
use.

Signed-off-by: Josh Law <josh2@disroot.org>
---
 arch/arm/cpu/armv8/sec_firmware.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c
index 44372cbe4a1..8c31fd19399 100644
--- a/arch/arm/cpu/armv8/sec_firmware.c
+++ b/arch/arm/cpu/armv8/sec_firmware.c
@@ -86,8 +86,8 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img,
 	const void *data;
 	size_t size;
 	ulong load;
-	const char *name, *str, *type;
-	int len;
+	const char *str, *type;
+	int count, i, len;
 
 	conf_node_off = fit_conf_get_node(sec_firmware_img, NULL);
 	if (conf_node_off < 0) {
@@ -104,16 +104,28 @@ static int sec_firmware_check_copy_loadable(const void *sec_firmware_img,
 
 	type = FIT_LOADABLE_PROP;
 
-	name = fdt_getprop(sec_firmware_img, conf_node_off, type, &len);
-	if (!name) {
+	count = fdt_stringlist_count(sec_firmware_img, conf_node_off, type);
+	if (count == -FDT_ERR_NOTFOUND) {
 		/* Loadables not present */
 		return 0;
 	}
+	if (count < 0) {
+		printf("SEC Firmware: invalid '%s' property: %s\n", type,
+		       fdt_strerror(count));
+		return -EINVAL;
+	}
 
 	printf("SEC Firmware: '%s' present in config\n", type);
 
-	for (str = name; str && ((str - name) < len);
-	     str = strchr(str, '\0') + 1) {
+	for (i = 0; i < count; i++) {
+		str = fdt_stringlist_get(sec_firmware_img, conf_node_off, type,
+					 i, &len);
+		if (!str) {
+			printf("SEC Firmware: can't read '%s' entry %d: %s\n",
+			       type, i, fdt_strerror(len));
+			return -EINVAL;
+		}
+
 		printf("%s: '%s'\n", type, str);
 		ld_node_off = fdt_subnode_offset(sec_firmware_img, images, str);
 		if (ld_node_off < 0) {
-- 
2.47.3