Binman can embed a copy of the image description into the images it builds as a fdtmap entry, but it omits the /binman/<image-name> 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 alpernebiyasak@gmail.com ---

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 18a7a3510548..a07a5888643a 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -775,7 +775,7 @@ def GetUniqueName(self): 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 876953f11324..e6f0159a229f 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5471,6 +5471,37 @@ def testFitSplitElfFaked(self): err, "Image '.*' is missing external blobs and is non-functional: .*")

+ 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 000000000000..6780d37f71f4 --- /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 000000000000..db63afb445e0 --- /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"; + }; + }; + }; +};

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 alpernebiyasak@gmail.com ---

tools/binman/control.py | 1 + tools/binman/etype/_testing.py | 36 +++++++++++++++++ tools/binman/ftest.py | 38 ++++++++++++++++++ .../binman/test/232_replace_with_bintool.dts | 39 +++++++++++++++++++ 4 files changed, 114 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 d4c8dc89201b..e170aeae4fab 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 5089de364294..696004878147 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 @@ def ReadNode(self): 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 @@ def ObtainContents(self, fake_size=0): 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 @@ def ProcessFdt(self, fdt): 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 e6f0159a229f..da9733d39a6a 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5502,6 +5502,44 @@ def testSafeUniqueNamesMulti(self):

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 000000000000..d7fabd2cd835 --- /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"; + }; + }; +};

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 alpernebiyasak@gmail.com ---

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 e170aeae4fab..ce57dc7efc7b 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 da9733d39a6a..3d79f82dff43 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5540,6 +5540,27 @@ def testReplaceCmdOtherWithBintool(self): 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()

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 alpernebiyasak@gmail.com ---

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 e0407715d819..035719871e04 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -384,7 +384,8 @@ def _add_entries(base_node, depth, node): 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 @@ def _add_node(base_node, depth, node):

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 @@ def _add_node(base_node, depth, node): # 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 @@ def SetImagePos(self, image_pos): 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 3d79f82dff43..87c072ef9c9c 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3761,6 +3761,13 @@ def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data): 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')