spl: binman: Check at runtime if binman symbols were filled in

Binman lets us declare symbols in SPL/TPL that refer to other entries in
the same binman image as them. These symbols are filled in with the
correct values while binman assembles the images, but this is done
in-memory only. Symbols marked as optional can be filled with
BINMAN_SYM_MISSING as an error value if their referred entry is missing.

However, the unmodified SPL/TPL binaries are still available on disk,
and can be used by people. For these files, nothing ensures that the
symbols are set to this error value, and they will be considered valid
when they are not.

Empirically, all symbols show up as zero in a sandbox_vpl build when we
run e.g. tpl/u-boot-tpl directly. On the other hand, zero is a perfectly
fine value for a binman-written symbol, so we cannot say the symbols
have wrong values based on that.

Declare a magic symbol that binman always fills in with a fixed value.
Check this value as an indicator that symbols were filled in correctly.
Return the error value for all symbols when this magic symbol has the
wrong value.

For binman tests, we need to make room for the new symbol in the mocked
SPL/TPL data by extending them by four bytes. This messes up some test
image layouts. Fix the affected values, and check the magic symbol
wherever it makes sense.

Signed-off-by: Alper Nebi Yasak <[email protected]>

13 files changed, 53 insertions, 36 deletions

diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index afa05e58fdd..6d440ddf21d 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -25,6 +25,9 @@ try:
 except:  # pragma: no cover
     ELF_TOOLS = False
 
+# BSYM in little endian, keep in sync with include/binman_sym.h
+BINMAN_SYM_MAGIC_VALUE = 0x4d595342
+
 # Information about an EFL symbol:
 # section (str): Name of the section containing this symbol
 # address (int): Address of the symbol (its value)
@@ -223,9 +226,12 @@ def LookupAndWriteSymbols(elf_fname, entry, section):
                 raise ValueError('%s has size %d: only 4 and 8 are supported' %
                                  (msg, sym.size))
 
-            # Look up the symbol in our entry tables.
-            value = section.GetImage().LookupImageSymbol(name, sym.weak, msg,
-                                                         base.address)
+            if name == '_binman_sym_magic':
+                value = BINMAN_SYM_MAGIC_VALUE
+            else:
+                # Look up the symbol in our entry tables.
+                value = section.GetImage().LookupImageSymbol(name, sym.weak,
+                                                             msg, base.address)
             if value is None:
                 value = -1
                 pack_string = pack_string.lower()
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index 02bc1083749..5a51c64cfee 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -127,7 +127,7 @@ class TestElf(unittest.TestCase):
         elf_fname = self.ElfTestFile('u_boot_binman_syms')
         with self.assertRaises(ValueError) as e:
             elf.LookupAndWriteSymbols(elf_fname, entry, section)
-        self.assertIn('entry_path has offset 4 (size 8) but the contents size '
+        self.assertIn('entry_path has offset 8 (size 8) but the contents size '
                       'is a', str(e.exception))
 
     def testMissingImageStart(self):
@@ -161,18 +161,20 @@ class TestElf(unittest.TestCase):
         This should produce -1 values for all thress symbols, taking up the
         first 16 bytes of the image.
         """
-        entry = FakeEntry(24)
+        entry = FakeEntry(28)
         section = FakeSection(sym_value=None)
         elf_fname = self.ElfTestFile('u_boot_binman_syms')
         elf.LookupAndWriteSymbols(elf_fname, entry, section)
-        self.assertEqual(tools.get_bytes(255, 20) + tools.get_bytes(ord('a'), 4),
-                                                                  entry.data)
+        expected = (struct.pack('<L', elf.BINMAN_SYM_MAGIC_VALUE) +
+                    tools.get_bytes(255, 20) +
+                    tools.get_bytes(ord('a'), 4))
+        self.assertEqual(expected, entry.data)
 
     def testDebug(self):
         """Check that enabling debug in the elf module produced debug output"""
         try:
             tout.init(tout.DEBUG)
-            entry = FakeEntry(20)
+            entry = FakeEntry(24)
             section = FakeSection()
             elf_fname = self.ElfTestFile('u_boot_binman_syms')
             with test_util.capture_sys_output() as (stdout, stderr):
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index b5cf549703a..fa1f421c052 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -43,8 +43,8 @@ from patman import tout
 # Contents of test files, corresponding to different entry types
 U_BOOT_DATA           = b'1234'
 U_BOOT_IMG_DATA       = b'img'
-U_BOOT_SPL_DATA       = b'56780123456789abcdefghi'
-U_BOOT_TPL_DATA       = b'tpl9876543210fedcbazyw'
+U_BOOT_SPL_DATA       = b'56780123456789abcdefghijklm'
+U_BOOT_TPL_DATA       = b'tpl9876543210fedcbazywvuts'
 BLOB_DATA             = b'89'
 ME_DATA               = b'0abcd'
 VGA_DATA              = b'vga'
@@ -1406,8 +1406,9 @@ class TestFunctional(unittest.TestCase):
         elf_fname = self.ElfTestFile('u_boot_binman_syms')
         syms = elf.GetSymbols(elf_fname, ['binman', 'image'])
         addr = elf.GetSymbolAddress(elf_fname, '__image_copy_start')
+        self.assertEqual(syms['_binman_sym_magic'].address, addr)
         self.assertEqual(syms['_binman_u_boot_spl_any_prop_offset'].address,
-                         addr)
+                         addr + 4)
 
         self._SetupSplElf('u_boot_binman_syms')
         data = self._DoReadFileDtb(dts, entry_args=entry_args,
@@ -1415,17 +1416,17 @@ class TestFunctional(unittest.TestCase):
         # The image should contain the symbols from u_boot_binman_syms.c
         # Note that image_pos is adjusted by the base address of the image,
         # which is 0x10 in our test image
-        sym_values = struct.pack('<LQLL', 0x00,
-                                 u_boot_offset + len(U_BOOT_DATA),
+        sym_values = struct.pack('<LLQLL', elf.BINMAN_SYM_MAGIC_VALUE,
+                                 0x00, u_boot_offset + len(U_BOOT_DATA),
                                  0x10 + u_boot_offset, 0x04)
-        expected = (sym_values + base_data[20:] +
+        expected = (sym_values + base_data[24:] +
                     tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values +
-                    base_data[20:])
+                    base_data[24:])
         self.assertEqual(expected, data)
 
     def testSymbols(self):
         """Test binman can assign symbols embedded in U-Boot"""
-        self.checkSymbols('053_symbols.dts', U_BOOT_SPL_DATA, 0x18)
+        self.checkSymbols('053_symbols.dts', U_BOOT_SPL_DATA, 0x1c)
 
     def testSymbolsNoDtb(self):
         """Test binman can assign symbols embedded in U-Boot SPL"""
@@ -3610,20 +3611,20 @@ class TestFunctional(unittest.TestCase):
 
     def _CheckSymbolsTplSection(self, dts, expected_vals):
         data = self._DoReadFile(dts)
-        sym_values = struct.pack('<LQLL', *expected_vals)
+        sym_values = struct.pack('<LLQLL', elf.BINMAN_SYM_MAGIC_VALUE, *expected_vals)
         upto1 = 4 + len(U_BOOT_SPL_DATA)
-        expected1 = tools.get_bytes(0xff, 4) + sym_values + U_BOOT_SPL_DATA[20:]
+        expected1 = tools.get_bytes(0xff, 4) + sym_values + U_BOOT_SPL_DATA[24:]
         self.assertEqual(expected1, data[:upto1])
 
         upto2 = upto1 + 1 + len(U_BOOT_SPL_DATA)
-        expected2 = tools.get_bytes(0xff, 1) + sym_values + U_BOOT_SPL_DATA[20:]
+        expected2 = tools.get_bytes(0xff, 1) + sym_values + U_BOOT_SPL_DATA[24:]
         self.assertEqual(expected2, data[upto1:upto2])
 
-        upto3 = 0x34 + len(U_BOOT_DATA)
+        upto3 = 0x3c + len(U_BOOT_DATA)
         expected3 = tools.get_bytes(0xff, 1) + U_BOOT_DATA
         self.assertEqual(expected3, data[upto2:upto3])
 
-        expected4 = sym_values + U_BOOT_TPL_DATA[20:]
+        expected4 = sym_values + U_BOOT_TPL_DATA[24:]
         self.assertEqual(expected4, data[upto3:upto3 + len(U_BOOT_TPL_DATA)])
 
     def testSymbolsTplSection(self):
@@ -3631,14 +3632,14 @@ class TestFunctional(unittest.TestCase):
         self._SetupSplElf('u_boot_binman_syms')
         self._SetupTplElf('u_boot_binman_syms')
         self._CheckSymbolsTplSection('149_symbols_tpl.dts',
-                                     [0x04, 0x1c, 0x10 + 0x34, 0x04])
+                                     [0x04, 0x20, 0x10 + 0x3c, 0x04])
 
     def testSymbolsTplSectionX86(self):
         """Test binman can assign symbols in a section with end-at-4gb"""
         self._SetupSplElf('u_boot_binman_syms_x86')
         self._SetupTplElf('u_boot_binman_syms_x86')
         self._CheckSymbolsTplSection('155_symbols_tpl_x86.dts',
-                                     [0xffffff04, 0xffffff1c, 0xffffff34,
+                                     [0xffffff04, 0xffffff20, 0xffffff3c,
                                       0x04])
 
     def testPackX86RomIfwiSectiom(self):
@@ -4488,7 +4489,7 @@ class TestFunctional(unittest.TestCase):
 
     def testSymbolsSubsection(self):
         """Test binman can assign symbols from a subsection"""
-        self.checkSymbols('187_symbols_sub.dts', U_BOOT_SPL_DATA, 0x18)
+        self.checkSymbols('187_symbols_sub.dts', U_BOOT_SPL_DATA, 0x1c)
 
     def testReadImageEntryArg(self):
         """Test reading an image that would need an entry arg to generate"""
diff --git a/tools/binman/test/021_image_pad.dts b/tools/binman/test/021_image_pad.dts
index 1ff8dab296f..c5abbbcdd6a 100644
--- a/tools/binman/test/021_image_pad.dts
+++ b/tools/binman/test/021_image_pad.dts
@@ -10,7 +10,7 @@
 		};
 
 		u-boot {
-			offset = <24>;
+			offset = <28>;
 		};
 	};
 };
diff --git a/tools/binman/test/024_sorted.dts b/tools/binman/test/024_sorted.dts
index b79d9adf682..b54f9b14191 100644
--- a/tools/binman/test/024_sorted.dts
+++ b/tools/binman/test/024_sorted.dts
@@ -7,7 +7,7 @@
 	binman {
 		sort-by-offset;
 		u-boot {
-			offset = <26>;
+			offset = <30>;
 		};
 
 		u-boot-spl {
diff --git a/tools/binman/test/028_pack_4gb_outside.dts b/tools/binman/test/028_pack_4gb_outside.dts
index 11a1f6059e2..b6ad7fb56a5 100644
--- a/tools/binman/test/028_pack_4gb_outside.dts
+++ b/tools/binman/test/028_pack_4gb_outside.dts
@@ -13,7 +13,7 @@
 		};
 
 		u-boot-spl {
-			offset = <0xffffffe7>;
+			offset = <0xffffffe3>;
 		};
 	};
 };
diff --git a/tools/binman/test/029_x86_rom.dts b/tools/binman/test/029_x86_rom.dts
index 88aa007bbae..ad8f9d6e1bd 100644
--- a/tools/binman/test/029_x86_rom.dts
+++ b/tools/binman/test/029_x86_rom.dts
@@ -7,13 +7,13 @@
 	binman {
 		sort-by-offset;
 		end-at-4gb;
-		size = <32>;
+		size = <36>;
 		u-boot {
-			offset = <0xffffffe0>;
+			offset = <0xffffffdc>;
 		};
 
 		u-boot-spl {
-			offset = <0xffffffe7>;
+			offset = <0xffffffe3>;
 		};
 	};
 };
diff --git a/tools/binman/test/053_symbols.dts b/tools/binman/test/053_symbols.dts
index 29658092764..b28f34a72fa 100644
--- a/tools/binman/test/053_symbols.dts
+++ b/tools/binman/test/053_symbols.dts
@@ -10,7 +10,7 @@
 		};
 
 		u-boot {
-			offset = <0x18>;
+			offset = <0x1c>;
 		};
 
 		u-boot-spl2 {
diff --git a/tools/binman/test/149_symbols_tpl.dts b/tools/binman/test/149_symbols_tpl.dts
index 0a4ab3f1fab..4e649c45978 100644
--- a/tools/binman/test/149_symbols_tpl.dts
+++ b/tools/binman/test/149_symbols_tpl.dts
@@ -11,12 +11,12 @@
 		};
 
 		u-boot-spl2 {
-			offset = <0x1c>;
+			offset = <0x20>;
 			type = "u-boot-spl";
 		};
 
 		u-boot {
-			offset = <0x34>;
+			offset = <0x3c>;
 		};
 
 		section {
diff --git a/tools/binman/test/155_symbols_tpl_x86.dts b/tools/binman/test/155_symbols_tpl_x86.dts
index 9d7dc51b3d9..e1ce33e67fb 100644
--- a/tools/binman/test/155_symbols_tpl_x86.dts
+++ b/tools/binman/test/155_symbols_tpl_x86.dts
@@ -14,12 +14,12 @@
 		};
 
 		u-boot-spl2 {
-			offset = <0xffffff1c>;
+			offset = <0xffffff20>;
 			type = "u-boot-spl";
 		};
 
 		u-boot {
-			offset = <0xffffff34>;
+			offset = <0xffffff3c>;
 		};
 
 		section {
diff --git a/tools/binman/test/187_symbols_sub.dts b/tools/binman/test/187_symbols_sub.dts
index 54511a73711..3ab62d37215 100644
--- a/tools/binman/test/187_symbols_sub.dts
+++ b/tools/binman/test/187_symbols_sub.dts
@@ -11,7 +11,7 @@
 			};
 
 			u-boot {
-				offset = <24>;
+				offset = <28>;
 			};
 		};
 
diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c
index 89fee5567e1..ed761246aec 100644
--- a/tools/binman/test/u_boot_binman_syms.c
+++ b/tools/binman/test/u_boot_binman_syms.c
@@ -5,9 +5,13 @@
  * Simple program to create some binman symbols. This is used by binman tests.
  */
 
+typedef unsigned long ulong;
+
 #include <linux/kconfig.h>
 #include <binman_sym.h>
 
+DECLARE_BINMAN_MAGIC_SYM;
+
 binman_sym_declare(unsigned long, u_boot_spl_any, offset);
 binman_sym_declare(unsigned long long, u_boot_spl2, offset);
 binman_sym_declare(unsigned long, u_boot_any, image_pos);
diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c
index c4a053f96f1..fa41b3d9a33 100644
--- a/tools/binman/test/u_boot_binman_syms_size.c
+++ b/tools/binman/test/u_boot_binman_syms_size.c
@@ -5,7 +5,11 @@
  * Simple program to create some binman symbols. This is used by binman tests.
  */
 
+typedef unsigned long ulong;
+
 #include <linux/kconfig.h>
 #include <binman_sym.h>
 
+DECLARE_BINMAN_MAGIC_SYM;
+
 binman_sym_declare(char, u_boot_spl, pos);