From 67bf2c8ded5b91ffa62e9aabededc9098810254f Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:44 +0300 Subject: [PATCH 1/9] binman: Fix unique names having '/.' for images read from files Binman can embed a copy of the image description into the images it builds as a fdtmap entry, but it omits the /binman/ prefix from the node paths while doing so. When reading an already-built image file, entries are reconstructed using this fdtmap and their associated nodes still lack that prefix. Some entries like fit and vblock create intermediate files whose names are based on an entry unique name. This name is constructed from their node's path by concatenating the parents with dots up to the binman node, e.g. /binman/image/foo/bar becomes 'image.foo.bar'. However, we don't have this /binman/image prefix when replacing entries in such an image. The /foo/bar entry we read when doing so erroneously has the unique name of '/.foo.bar', causing permission errors when the entry attempts to create files based on that. Fix the unique-name generation by stopping at the '/' node like how it stops at the binman node. As the unique names are used as filenames, add tests that check if they're safe to use as filenames. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/entry.py | 2 +- tools/binman/ftest.py | 31 ++++++++++++++++ tools/binman/test/230_unique_names.dts | 34 ++++++++++++++++++ tools/binman/test/231_unique_names_multi.dts | 38 ++++++++++++++++++++ 4 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/230_unique_names.dts create mode 100644 tools/binman/test/231_unique_names_multi.dts diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 18a7a351054..a07a5888643 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -775,7 +775,7 @@ features to produce new behaviours. node = self._node while node.parent: node = node.parent - if node.name == 'binman': + if node.name in ('binman', '/'): break name = '%s.%s' % (node.name, name) return name diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 4ce181a0666..94c4389b1b5 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5523,5 +5523,36 @@ fdt fdtmap Extract the devicetree blob from the fdtmap with self.assertRaises(ValueError) as e: data = self._DoReadFile('231_pre_load_invalid_key.dts') + def _CheckSafeUniqueNames(self, *images): + """Check all entries of given images for unsafe unique names""" + for image in images: + entries = {} + image._CollectEntries(entries, {}, image) + for entry in entries.values(): + uniq = entry.GetUniqueName() + + # Used as part of a filename, so must not be absolute paths. + self.assertFalse(os.path.isabs(uniq)) + + def testSafeUniqueNames(self): + """Test entry unique names are safe in single image configuration""" + data = self._DoReadFileRealDtb('230_unique_names.dts') + + orig_image = control.images['image'] + image_fname = tools.get_output_filename('image.bin') + image = Image.FromFile(image_fname) + + self._CheckSafeUniqueNames(orig_image, image) + + def testSafeUniqueNamesMulti(self): + """Test entry unique names are safe with multiple images""" + data = self._DoReadFileRealDtb('231_unique_names_multi.dts') + + orig_image = control.images['image'] + image_fname = tools.get_output_filename('image.bin') + image = Image.FromFile(image_fname) + + self._CheckSafeUniqueNames(orig_image, image) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/230_unique_names.dts b/tools/binman/test/230_unique_names.dts new file mode 100644 index 00000000000..6780d37f71f --- /dev/null +++ b/tools/binman/test/230_unique_names.dts @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0xc00>; + allow-repack; + + u-boot { + }; + + fdtmap { + }; + + u-boot2 { + type = "u-boot"; + }; + + text { + text = "some text"; + }; + + u-boot-dtb { + }; + + image-header { + location = "end"; + }; + }; +}; diff --git a/tools/binman/test/231_unique_names_multi.dts b/tools/binman/test/231_unique_names_multi.dts new file mode 100644 index 00000000000..db63afb445e --- /dev/null +++ b/tools/binman/test/231_unique_names_multi.dts @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + multiple-images; + + image { + size = <0xc00>; + allow-repack; + + u-boot { + }; + + fdtmap { + }; + + u-boot2 { + type = "u-boot"; + }; + + text { + text = "some text"; + }; + + u-boot-dtb { + }; + + image-header { + location = "end"; + }; + }; + }; +}; From 8ee4ec9bf560dd511f3bfecd5df254fb6814ef67 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:45 +0300 Subject: [PATCH 2/9] binman: Collect bintools for images when replacing entries Binman entries can use other executables to compute their data, usually in their ObtainContents() methods. Subclasses of Entry_section would use bintools in their BuildSectionData() method instead, which is called from several places including their Pack(). These binary tools are resolved correctly while building an image from a device-tree description so that they can be used from these methods. However, this is not being done when replacing entries in an image, which can result in an error as the Pack() methods attempt to use them. Collect and resolve entries' bintools also when replacing entries to fix Pack() errors. Add a way to mock bintool usage in the testing entry type and tests that check bintools are being resolved for such an entry. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/control.py | 1 + tools/binman/etype/_testing.py | 36 +++++++++++++++++ tools/binman/ftest.py | 39 +++++++++++++++++++ .../binman/test/232_replace_with_bintool.dts | 39 +++++++++++++++++++ 4 files changed, 115 insertions(+) create mode 100644 tools/binman/test/232_replace_with_bintool.dts diff --git a/tools/binman/control.py b/tools/binman/control.py index d4c8dc89201..e170aeae4fa 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -299,6 +299,7 @@ def BeforeReplace(image, allow_resize): """ state.PrepareFromLoadedData(image) image.LoadData() + image.CollectBintools() # If repacking, drop the old offset/size values except for the original # ones, so we are only left with the constraints. diff --git a/tools/binman/etype/_testing.py b/tools/binman/etype/_testing.py index 5089de36429..69600487814 100644 --- a/tools/binman/etype/_testing.py +++ b/tools/binman/etype/_testing.py @@ -39,6 +39,10 @@ class Entry__testing(Entry): error if not) force-bad-datatype: Force a call to GetEntryArgsOrProps() with a bad data type (generating an error) + require-bintool-for-contents: Raise an error if the specified + bintool isn't usable in ObtainContents() + require-bintool-for-pack: Raise an error if the specified + bintool isn't usable in Pack() """ def __init__(self, section, etype, node): super().__init__(section, etype, node) @@ -82,6 +86,26 @@ class Entry__testing(Entry): self.return_contents = True self.contents = b'aa' + # Set to the required bintool when collecting bintools. + self.bintool_for_contents = None + self.require_bintool_for_contents = fdt_util.GetString(self._node, + 'require-bintool-for-contents') + if self.require_bintool_for_contents == '': + self.require_bintool_for_contents = '_testing' + + self.bintool_for_pack = None + self.require_bintool_for_pack = fdt_util.GetString(self._node, + 'require-bintool-for-pack') + if self.require_bintool_for_pack == '': + self.require_bintool_for_pack = '_testing' + + def Pack(self, offset): + """Figure out how to pack the entry into the section""" + if self.require_bintool_for_pack: + if self.bintool_for_pack is None: + self.Raise("Required bintool unusable in Pack()") + return super().Pack(offset) + def ObtainContents(self, fake_size=0): if self.return_unknown_contents or not self.return_contents: return False @@ -92,6 +116,9 @@ class Entry__testing(Entry): self.contents_size = len(self.data) if self.return_contents_once: self.return_contents = False + if self.require_bintool_for_contents: + if self.bintool_for_contents is None: + self.Raise("Required bintool unusable in ObtainContents()") return True def GetOffsets(self): @@ -127,3 +154,12 @@ class Entry__testing(Entry): if not self.never_complete_process_fdt: self.process_fdt_ready = True return ready + + def AddBintools(self, btools): + """Add the bintools used by this entry type""" + if self.require_bintool_for_contents is not None: + self.bintool_for_contents = self.AddBintool(btools, + self.require_bintool_for_contents) + if self.require_bintool_for_pack is not None: + self.bintool_for_pack = self.AddBintool(btools, + self.require_bintool_for_pack) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 94c4389b1b5..03e6d9233b2 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5554,5 +5554,44 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self._CheckSafeUniqueNames(orig_image, image) + def testReplaceCmdWithBintool(self): + """Test replacing an entry that needs a bintool to pack""" + data = self._DoReadFileRealDtb('232_replace_with_bintool.dts') + expected = U_BOOT_DATA + b'aa' + self.assertEqual(expected, data[:len(expected)]) + + try: + tmpdir, updated_fname = self._SetupImageInTmpdir() + fname = os.path.join(tmpdir, 'update-testing.bin') + tools.write_file(fname, b'zz') + self._DoBinman('replace', '-i', updated_fname, + '_testing', '-f', fname) + + data = tools.read_file(updated_fname) + expected = U_BOOT_DATA + b'zz' + self.assertEqual(expected, data[:len(expected)]) + finally: + shutil.rmtree(tmpdir) + + def testReplaceCmdOtherWithBintool(self): + """Test replacing an entry when another needs a bintool to pack""" + data = self._DoReadFileRealDtb('232_replace_with_bintool.dts') + expected = U_BOOT_DATA + b'aa' + self.assertEqual(expected, data[:len(expected)]) + + try: + tmpdir, updated_fname = self._SetupImageInTmpdir() + fname = os.path.join(tmpdir, 'update-u-boot.bin') + tools.write_file(fname, b'x' * len(U_BOOT_DATA)) + self._DoBinman('replace', '-i', updated_fname, + 'u-boot', '-f', fname) + + data = tools.read_file(updated_fname) + expected = b'x' * len(U_BOOT_DATA) + b'aa' + self.assertEqual(expected, data[:len(expected)]) + finally: + shutil.rmtree(tmpdir) + + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/232_replace_with_bintool.dts b/tools/binman/test/232_replace_with_bintool.dts new file mode 100644 index 00000000000..d7fabd2cd83 --- /dev/null +++ b/tools/binman/test/232_replace_with_bintool.dts @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <0xc00>; + allow-repack; + + u-boot { + }; + + _testing { + require-bintool-for-contents; + require-bintool-for-pack; + }; + + fdtmap { + }; + + u-boot2 { + type = "u-boot"; + }; + + text { + text = "some text"; + }; + + u-boot-dtb { + }; + + image-header { + location = "end"; + }; + }; +}; From e2ce4fb9869dcedd3ecb4ad552c86e02248649df Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:46 +0300 Subject: [PATCH 3/9] binman: Don't reset offset/size if image doesn't allow repacking When an image has the 'allow-repack' property, binman includes the original offset and size properties from the image description in the fdtmap. These are later used as the packing constraints when replacing entries in an image, so other unconstrained entries can be freely positioned. Replacing an entry in an image without 'allow-repack' (and therefore the original offsets) follows the same logic and results in entries being merely concatenated. Instead, skip resetting the calculated offsets and sizes to the missing originals for these images so that every entry is constrained to its existing offset/size. Add tests that replace an entry with smaller or equal-sized data, in an image that doesn't allow repacking. Attempting to do so with bigger-size data is already an error that is already being tested. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/control.py | 2 +- tools/binman/ftest.py | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/tools/binman/control.py b/tools/binman/control.py index e170aeae4fa..ce57dc7efc7 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -303,7 +303,7 @@ def BeforeReplace(image, allow_resize): # If repacking, drop the old offset/size values except for the original # ones, so we are only left with the constraints. - if allow_resize: + if image.allow_repack and allow_resize: image.ResetForPack() diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 03e6d9233b2..c9a82094c56 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5592,6 +5592,27 @@ fdt fdtmap Extract the devicetree blob from the fdtmap finally: shutil.rmtree(tmpdir) + def testReplaceResizeNoRepackSameSize(self): + """Test replacing entries with same-size data without repacking""" + expected = b'x' * len(U_BOOT_DATA) + data, expected_fdtmap, _ = self._RunReplaceCmd('u-boot', expected) + self.assertEqual(expected, data) + + path, fdtmap = state.GetFdtContents('fdtmap') + self.assertIsNotNone(path) + self.assertEqual(expected_fdtmap, fdtmap) + + def testReplaceResizeNoRepackSmallerSize(self): + """Test replacing entries with smaller-size data without repacking""" + new_data = b'x' + data, expected_fdtmap, _ = self._RunReplaceCmd('u-boot', new_data) + expected = new_data.ljust(len(U_BOOT_DATA), b'\0') + self.assertEqual(expected, data) + + path, fdtmap = state.GetFdtContents('fdtmap') + self.assertIsNotNone(path) + self.assertEqual(expected_fdtmap, fdtmap) + if __name__ == "__main__": unittest.main() From e736878b0865cdf963f02ec088b1df1d600ded20 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:47 +0300 Subject: [PATCH 4/9] binman: Remove '/images/' fragment from FIT subentry paths Binman FIT entry nodes describe their subentries in an 'images' subnode, same as how they would be written for the mkimage executable. The entry type initially manually managed its subentries keyed by their node paths relative to its base node. It was later converted to a proper section while still keeping the same keys for subentries. These subentry keys of sections are used as path fragments, so they must not contain the path separator character '/'. Otherwise, they won't be addressable by binman extract/replace commands. Change these keys from the '/images/foo' forms to the subentry node names. Extend the simple FIT tests to check for this. Signed-off-by: Alper Nebi Yasak --- tools/binman/etype/fit.py | 13 ++++++++----- tools/binman/ftest.py | 7 +++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index e0407715d81..035719871e0 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -384,7 +384,8 @@ class Entry_fit(Entry_section): entry.ReadNode() # The hash subnodes here are for mkimage, not binman. entry.SetUpdateHash(False) - self._entries[rel_path] = entry + image_name = rel_path[len('/images/'):] + self._entries[image_name] = entry for subnode in node.subnodes: _add_entries(base_node, depth + 1, subnode) @@ -630,7 +631,8 @@ class Entry_fit(Entry_section): has_images = depth == 2 and in_images if has_images: - entry = self._priv_entries[rel_path] + image_name = rel_path[len('/images/'):] + entry = self._priv_entries[image_name] data = entry.GetData() fsw.property('data', bytes(data)) @@ -643,12 +645,12 @@ class Entry_fit(Entry_section): # fsw.add_node() or _add_node() for it. pass elif self.GetImage().generate and subnode.name.startswith('@'): - entry = self._priv_entries.get(subnode_path) + entry = self._priv_entries.get(subnode.name) _gen_node(base_node, subnode, depth, in_images, entry) # This is a generator (template) entry, so remove it from # the list of entries used by PackEntries(), etc. Otherwise # it will appear in the binman output - to_remove.append(subnode_path) + to_remove.append(subnode.name) else: with fsw.add_node(subnode.name): _add_node(base_node, depth + 1, subnode) @@ -693,7 +695,8 @@ class Entry_fit(Entry_section): fdt = Fdt.FromData(self.GetData()) fdt.Scan() - for path, section in self._entries.items(): + for image_name, section in self._entries.items(): + path = f"/images/{image_name}" node = fdt.GetNode(path) data_prop = node.props.get("data") diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index c9a82094c56..0e77358b32b 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3764,6 +3764,13 @@ class TestFunctional(unittest.TestCase): self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0])) self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0])) + # Check if entry listing correctly omits /images/ + image = control.images['image'] + fit_entry = image.GetEntries()['fit'] + subentries = list(fit_entry.GetEntries().keys()) + expected = ['kernel', 'fdt-1'] + self.assertEqual(expected, subentries) + def testSimpleFit(self): """Test an image with a FIT inside""" data = self._DoReadFile('161_fit.dts') From 74d3b2311deeefe0da8df75d16b8d839ba5ff2a4 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:48 +0300 Subject: [PATCH 5/9] binman: Create FIT subentries in the FIT section, not its parent When reading images from a file, each entry's data is read from its parent section as specified in the Entry.Create() call that created it. The FIT entry type has been creating its subentries under its parent (their grandparent), as creating them under the FIT entry resulted in an error until FIT was converted into a proper section. FIT subentries have their offsets relative to the FIT section, and reading those offsets in the parent section results in wrong data. The subentries rightfully belong under the FIT entries, so create them there. Add tests checking that we can extract the correct data for a FIT entry and its subentries. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/etype/fit.py | 2 +- tools/binman/ftest.py | 35 +++++++++ tools/binman/test/233_fit_extract_replace.dts | 74 +++++++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/233_fit_extract_replace.dts diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 035719871e0..12306623af6 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -380,7 +380,7 @@ class Entry_fit(Entry_section): # section entries for them here to merge the content subnodes # together and put the merged contents in the subimage node's # 'data' property later. - entry = Entry.Create(self.section, node, etype='section') + entry = Entry.Create(self, node, etype='section') entry.ReadNode() # The hash subnodes here are for mkimage, not binman. entry.SetUpdateHash(False) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 0e77358b32b..3fe57532d2b 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5620,6 +5620,41 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertIsNotNone(path) self.assertEqual(expected_fdtmap, fdtmap) + def testExtractFit(self): + """Test extracting a FIT section""" + self._DoReadFileRealDtb('233_fit_extract_replace.dts') + image_fname = tools.get_output_filename('image.bin') + + fit_data = control.ReadEntry(image_fname, 'fit') + fit = fdt.Fdt.FromData(fit_data) + fit.Scan() + + # Check subentry data inside the extracted fit + for node_path, expected in [ + ('/images/kernel', U_BOOT_DATA), + ('/images/fdt-1', U_BOOT_NODTB_DATA), + ('/images/scr-1', COMPRESS_DATA), + ]: + node = fit.GetNode(node_path) + data = fit.GetProps(node)['data'].bytes + self.assertEqual(expected, data) + + def testExtractFitSubentries(self): + """Test extracting FIT section subentries""" + self._DoReadFileRealDtb('233_fit_extract_replace.dts') + image_fname = tools.get_output_filename('image.bin') + + for entry_path, expected in [ + ('fit/kernel', U_BOOT_DATA), + ('fit/kernel/u-boot', U_BOOT_DATA), + ('fit/fdt-1', U_BOOT_NODTB_DATA), + ('fit/fdt-1/u-boot-nodtb', U_BOOT_NODTB_DATA), + ('fit/scr-1', COMPRESS_DATA), + ('fit/scr-1/blob', COMPRESS_DATA), + ]: + data = control.ReadEntry(image_fname, entry_path) + self.assertEqual(expected, data) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/233_fit_extract_replace.dts b/tools/binman/test/233_fit_extract_replace.dts new file mode 100644 index 00000000000..b44d05afe1a --- /dev/null +++ b/tools/binman/test/233_fit_extract_replace.dts @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + allow-repack; + + fill { + size = <0x1000>; + fill-byte = [77]; + }; + + fit { + description = "test-desc"; + #address-cells = <1>; + + images { + kernel { + description = "test u-boot"; + type = "kernel"; + arch = "arm64"; + os = "linux"; + compression = "none"; + load = <00000000>; + entry = <00000000>; + + u-boot { + }; + }; + + fdt-1 { + description = "test u-boot-nodtb"; + type = "flat_dt"; + arch = "arm64"; + compression = "none"; + + u-boot-nodtb { + }; + }; + + scr-1 { + description = "test blob"; + type = "script"; + arch = "arm64"; + compression = "none"; + + blob { + filename = "compress"; + }; + }; + }; + + configurations { + default = "conf-1"; + + conf-1 { + description = "Kernel with FDT blob"; + kernel = "kernel"; + fdt = "fdt-1"; + }; + }; + }; + + u-boot-dtb { + }; + + fdtmap { + }; + }; +}; From 99283e5389cd5b8b7bb191913fa1d3d18e4bfbec Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:49 +0300 Subject: [PATCH 6/9] binman: Test replacing non-section entries in FIT subsections A previous patch fixes binman to correctly extract FIT subentries. This makes it easier to test replacing these entries as we can write tests using an existing helper function that relies on extracting the replaced entry. Add tests that replace leaf entries in FIT subsections with data of various sizes. Replacing the subsections or the whole FIT section does not work yet due to the section contents being re-built from unreplaced subentries' data. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/ftest.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 3fe57532d2b..421754b1d0a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5655,6 +5655,44 @@ fdt fdtmap Extract the devicetree blob from the fdtmap data = control.ReadEntry(image_fname, entry_path) self.assertEqual(expected, data) + def testReplaceFitSubentryLeafSameSize(self): + """Test replacing a FIT leaf subentry with same-size data""" + new_data = b'x' * len(U_BOOT_DATA) + data, expected_fdtmap, _ = self._RunReplaceCmd( + 'fit/kernel/u-boot', new_data, + dts='233_fit_extract_replace.dts') + self.assertEqual(new_data, data) + + path, fdtmap = state.GetFdtContents('fdtmap') + self.assertIsNotNone(path) + self.assertEqual(expected_fdtmap, fdtmap) + + def testReplaceFitSubentryLeafBiggerSize(self): + """Test replacing a FIT leaf subentry with bigger-size data""" + new_data = b'ub' * len(U_BOOT_NODTB_DATA) + data, expected_fdtmap, _ = self._RunReplaceCmd( + 'fit/fdt-1/u-boot-nodtb', new_data, + dts='233_fit_extract_replace.dts') + self.assertEqual(new_data, data) + + # Will be repacked, so fdtmap must change + path, fdtmap = state.GetFdtContents('fdtmap') + self.assertIsNotNone(path) + self.assertNotEqual(expected_fdtmap, fdtmap) + + def testReplaceFitSubentryLeafSmallerSize(self): + """Test replacing a FIT leaf subentry with smaller-size data""" + new_data = b'x' + expected = new_data.ljust(len(U_BOOT_NODTB_DATA), b'\0') + data, expected_fdtmap, _ = self._RunReplaceCmd( + 'fit/fdt-1/u-boot-nodtb', new_data, + dts='233_fit_extract_replace.dts') + self.assertEqual(expected, data) + + path, fdtmap = state.GetFdtContents('fdtmap') + self.assertIsNotNone(path) + self.assertEqual(expected_fdtmap, fdtmap) + if __name__ == "__main__": unittest.main() From 82337bb6b63226c9a8a78dc03a1af1eab6494a6b Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Sun, 27 Mar 2022 18:31:50 +0300 Subject: [PATCH 7/9] binman: Refuse to replace sections for now Binman interfaces allow attempts to replace any entry in the image with arbitrary data. When trying to replace sections, the changes in the section entry's data are not propagated to its child entries. This, combined with how sections rebuild their contents from its children, eventually causes the replaced contents to be silently overwritten by rebuilt contents equivalent to the original data. Add a simple test for replacing a section that is currently failing due to this behaviour, and mark it as an expected failure. Also, raise an error when replacing a section instead of silently pretending it was replaced. Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- tools/binman/etype/section.py | 3 +++ tools/binman/ftest.py | 9 ++++++++ .../test/234_replace_section_simple.dts | 23 +++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 tools/binman/test/234_replace_section_simple.dts diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index ccac658c183..bd67238b919 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -788,6 +788,9 @@ class Entry_section(Entry): data = new_data return data + def WriteData(self, data, decomp=True): + self.Raise("Replacing sections is not implemented yet") + def WriteChildData(self, child): return True diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 421754b1d0a..b5cf549703a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5693,6 +5693,15 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertIsNotNone(path) self.assertEqual(expected_fdtmap, fdtmap) + @unittest.expectedFailure + def testReplaceSectionSimple(self): + """Test replacing a simple section with arbitrary data""" + new_data = b'w' * len(COMPRESS_DATA + U_BOOT_DATA) + data, expected_fdtmap, _ = self._RunReplaceCmd( + 'section', new_data, + dts='234_replace_section_simple.dts') + self.assertEqual(new_data, data) + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/234_replace_section_simple.dts b/tools/binman/test/234_replace_section_simple.dts new file mode 100644 index 00000000000..c9d5c328561 --- /dev/null +++ b/tools/binman/test/234_replace_section_simple.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/; + +/ { + binman { + allow-repack; + + u-boot-dtb { + }; + + section { + blob { + filename = "compress"; + }; + + u-boot { + }; + }; + + fdtmap { + }; + }; +}; From 06c73e37b329f6cc564b766cc739dff3c909e8c9 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Fri, 22 Apr 2022 18:25:47 +0300 Subject: [PATCH 8/9] MAINTAINERS: Add Alper as a binman maintainer I ended up learning most of binman internals while trying to add a few features to it, and I recently started reviewing binman series that would not affect me personally. I'll keep working on it and try to do more reviews. Add myself as a maintainer for binman. Signed-off-by: Alper Nebi Yasak Acked-by: Tom Rini --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index f7665fccf7a..8e49a84bad4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -690,6 +690,7 @@ F: arch/arm/dts/phytium-durian.dts BINMAN M: Simon Glass +M: Alper Nebi Yasak S: Maintained F: tools/binman/ From dd2e8ed41555b1f9e139c7be3a073885684670df Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 Apr 2022 14:53:18 +0200 Subject: [PATCH 9/9] binman: don't import deprecated distutils package 'make tests' fails on Ubuntu 22.04 with: binman: ./tools/binman/binman:12: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives from distutils.sysconfig import get_python_lib ./tools/binman/binman:12: DeprecationWarning: The distutils.sysconfig module is deprecated, use sysconfig instead from distutils.sysconfig import get_python_lib AssertionError: 0 != 468 As we don't use Ubuntu 16.04 for our CI anymore drop the import. Signed-off-by: Heinrich Schuchardt Reviewed-by: Alper Nebi Yasak --- tools/binman/main.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tools/binman/main.py b/tools/binman/main.py index 9392b59adb1..5fb9404ef6a 100755 --- a/tools/binman/main.py +++ b/tools/binman/main.py @@ -9,7 +9,6 @@ """See README for more information""" -from distutils.sysconfig import get_python_lib import os import site import sys @@ -44,12 +43,6 @@ sys.path.insert(2, os.path.join(srctree, 'scripts/dtc/pylibfdt')) sys.path.insert(2, os.path.join(srctree, 'build-sandbox/scripts/dtc/pylibfdt')) sys.path.insert(2, os.path.join(srctree, 'build-sandbox_spl/scripts/dtc/pylibfdt')) -# When running under python-coverage on Ubuntu 16.04, the dist-packages -# directories are dropped from the python path. Add them in so that we can find -# the elffile module. We could use site.getsitepackages() here but unfortunately -# that is not available in a virtualenv. -sys.path.append(get_python_lib()) - from binman import cmdline from binman import control from patman import test_util