Improved allocation of .bss(-like) sections

ofrak_core/ofrak/core/patch_maker/modifiers.py

@@ -21,6 +21,7 @@
 from ofrak_patch_maker.toolchain.abstract import Toolchain
 from ofrak_patch_maker.toolchain.model import Segment, ToolchainConfig
 from ofrak_type.memory_permissions import MemoryPermissions
+from ofrak_type.error import NotFoundError
 
 LOGGER = logging.getLogger(__file__)
 
@@ -190,7 +191,15 @@ def from_fem(fem: FEM) -> "SegmentInjectorModifierConfig":
         for segment in fem.executable.segments:
             if segment.length == 0:
                 continue
-            segment_data = exe_data[segment.offset : segment.offset + segment.length]
+
+            if segment.is_bss:
+                # It's possible for NOBITS sections like .bss to be allocated into RW MemoryRegions
+                # that are mapped to data. In that case, we should zero out the region instead
+                # of patching the arbitrary data in the FEM
+                segment_data = b"\0" * segment.length
+            else:
+                segment_data = exe_data[segment.offset : segment.offset + segment.length]
+
             extracted_segments.append((segment, segment_data))
         return SegmentInjectorModifierConfig(tuple(extracted_segments))
 
@@ -221,15 +230,14 @@ async def modify(self, resource: Resource, config: SegmentInjectorModifierConfig
         injection_tasks: List[Tuple[Resource, BinaryInjectorModifierConfig]] = []
 
         for segment, segment_data in config.segments_and_data:
-            if segment.length == 0 or segment.vm_address == 0:
+            if segment.length == 0 or not segment.is_allocated:
                 continue
             if segment.length > 0:
                 LOGGER.debug(
                     f"    Segment {segment.segment_name} - {segment.length} "
                     f"bytes @ {hex(segment.vm_address)}",
                 )
-            if segment.segment_name.startswith(".bss"):
-                continue
+
             if segment.segment_name.startswith(".rela"):
                 continue
             if segment.segment_name.startswith(".got"):
@@ -239,18 +247,31 @@ async def modify(self, resource: Resource, config: SegmentInjectorModifierConfig
                 # See PatchFromSourceModifier
                 continue
 
-            patches = [(segment.vm_address, segment_data)]
-            region = MemoryRegion.get_mem_region_with_vaddr_from_sorted(
-                segment.vm_address, sorted_regions
-            )
-            if region is None:
+            try:
+                region = MemoryRegion.get_mem_region_with_vaddr_from_sorted(
+                    segment.vm_address, sorted_regions
+                )
+            except NotFoundError:
+                # uninitialized section like .bss mapped to arbitrary memory range without corresponding
+                # MemoryRegion resource, no patch needed.
+                if segment.is_bss:
+                    continue
+                raise
+
+            region_mapped_to_data = region.resource.get_data_id() is not None
+            if region_mapped_to_data:
+                patches = [(segment.vm_address, segment_data)]
+                injection_tasks.append((region.resource, BinaryInjectorModifierConfig(patches)))
+            else:
+                if segment.is_bss:
+                    # uninitialized section like .bss mapped to arbitrary memory range without corresponding
+                    # data on a resource, no patch needed.
+                    continue
                 raise ValueError(
                     f"Cannot inject patch because the memory region at vaddr "
-                    f"{hex(segment.vm_address)} is None"
+                    f"{hex(segment.vm_address)} is not mapped to data"
                 )
 
-            injection_tasks.append((region.resource, BinaryInjectorModifierConfig(patches)))
-
         for injected_resource, injection_config in injection_tasks:
             result = await injected_resource.run(BinaryInjectorModifier, injection_config)
             # The above can patch data of any of injected_resources' descendants or ancestors

ofrak_patch_maker/ofrak_patch_maker/binary_parser/gnu.py

@@ -26,7 +26,7 @@ class GNU_ELF_Parser(AbstractBinaryFileParser):
         r"(?P<vma>[0-9A-Fa-f]+)[ \t]+"
         r"(?P<lma>[0-9A-Fa-f]+)[ \t]+"
         r"(?P<offset>[0-9A-Fa-f]+)[ \t]+"
-        r"(?P<alignment>[0-9]\*\*[0-9])\n[ \n]+"
+        r"2\*\*(?P<alignment>[0-9])\n[ \n]+"
         r"(?P<flags>[\S, ]+)",
         flags=re.MULTILINE,
     )
@@ -62,13 +62,19 @@ def parse_sections(self, output: str) -> Tuple[Segment, ...]:
                 permissions = permissions + MemoryPermissions.X
             # TODO: Figure out how to infer this.
             is_entry = False
+
+            is_allocated = "ALLOC" in section_data.group("flags")
+            is_bss = is_allocated and "LOAD" not in section_data.group("flags")
             seg = Segment(
                 segment_name=section_data.group("name"),
                 vm_address=int(section_data.group("vma"), 16),
                 offset=int(section_data.group("offset"), 16),
                 is_entry=is_entry,
                 length=int(section_data.group("size"), 16),
                 access_perms=permissions,
+                is_allocated=is_allocated,
+                is_bss=is_bss,
+                alignment=1 << int(section_data.group("alignment")),
             )
             segments.append(seg)
         return tuple(segments)

ofrak_patch_maker/ofrak_patch_maker/binary_parser/llvm.py

@@ -57,26 +57,32 @@ def _parse_readobj_section(lines: List[str], keys: Dict[str, str], flag_key: str
                 else:
                     kvs[remaining_keys[flag_key]] = MemoryPermissions.R
 
+                kvs["is_allocated"] = flags & 0x02 == 0x02
+
                 del remaining_keys[flag_key]
 
             elif ": " in line:
+                value: Union[str, int, bool]
                 key, value = tuple(line.strip().split(": "))
 
                 if key in remaining_keys:
                     # Only take the value text until the first whitespace.
                     value = value.split(" ")[0]
 
-                    # Try to convert the value to an integer.
-                    try:
-                        value = int(value, 0)  # type: ignore
-                    except ValueError:
-                        pass
+                    if key == "Type":
+                        value = value == "SHT_NOBITS"
+                    else:
+                        # Try to convert the value to an integer.
+                        try:
+                            value = int(value, 0)
+                        except ValueError:
+                            pass
 
                     kvs[remaining_keys[key]] = value
                     del remaining_keys[key]
 
-                    if len(remaining_keys) == 0:
-                        break
+            if len(remaining_keys) == 0:
+                break
 
         if len(remaining_keys) > 0:
             raise ToolchainException("Could not parse all keys!")
@@ -104,6 +110,8 @@ def parse_sections(self, output: str) -> Tuple[Segment, ...]:
             "Offset": "offset",
             "Size": "length",
             "Flags": "access_perms",
+            "Type": "is_bss",
+            "AddressAlignment": "alignment",
         }
 
         return self._parse_readobj_sections(output, section_keys, "Flags")

ofrak_patch_maker/ofrak_patch_maker/model.py

@@ -6,6 +6,7 @@
 from dataclasses import dataclass
 from enum import Enum
 from typing import Mapping, Optional, Set, Tuple
+from warnings import warn
 
 from ofrak_patch_maker.toolchain.model import Segment, BinFileType
 from ofrak_type.symbol_type import LinkableSymbolType
@@ -34,15 +35,19 @@ class AssembledObject:
     :var segment_map: e.g. `{".text", Segment(...)}`
     :var strong_symbols:
     :var unresolved_symbols: {symbol name: (address, symbol type)}
-    :var bss_size_required:
+    :var bss_size_required: DEPRECATED
     """
 
     path: str
     file_format: BinFileType
     segment_map: Mapping[str, Segment]  # segment name to Segment
     strong_symbols: Mapping[str, Tuple[int, LinkableSymbolType]]
     unresolved_symbols: Mapping[str, Tuple[int, LinkableSymbolType]]
-    bss_size_required: int
+    bss_size_required: Optional[int] = None
+
+    def __post_init__(self):
+        if self.bss_size_required is not None:
+            warn("AssembledObject.bss_size_required is deprecated")
 
 
 @dataclass(frozen=True)
@@ -115,17 +120,21 @@ class BOM:
     :var name: a name
     :var object_map: {source file path: AssembledObject}
     :var unresolved_symbols: symbols used but undefined within the BOM source files
-    :var bss_size_required:
+    :var bss_size_required: DEPRECATED
     :var entry_point_symbol: symbol of the patch entrypoint, when relevant
     """
 
     name: str
     object_map: Mapping[str, AssembledObject]
     unresolved_symbols: Set[str]
-    bss_size_required: int
+    bss_size_required: Optional[int]
     entry_point_symbol: Optional[str]
     segment_alignment: int
 
+    def __post_init__(self):
+        if self.bss_size_required is not None:
+            warn("BOM.bss_size_required is deprecated")
+
 
 class SourceFileType(Enum):
     DEFAULT = 0

ofrak_patch_maker/ofrak_patch_maker/patch_maker.py

@@ -151,24 +151,17 @@ def prepare_object(self, object_path: str) -> AssembledObject:
         # Symbols defined in another file which may or may not be another patch source file or the target binary.
         relocation_symbols = self._toolchain.get_bin_file_rel_symbols(object_path)
 
-        bss_size_required = 0
         segment_map = {}
         for s in segments:
-            if self._toolchain.keep_section(s.segment_name):
+            if s.is_allocated:
                 segment_map[s.segment_name] = s
-            if s.segment_name.startswith(".bss"):
-                if s.length > 0 and self._toolchain._config.no_bss_section:
-                    raise PatchMakerException(
-                        f"{s.segment_name} found but `no_bss_section` is set in the provided ToolchainConfig!"
-                    )
-                bss_size_required += s.length
+
         return AssembledObject(
             object_path,
             self._toolchain.file_format,
             immutabledict(segment_map),
             immutabledict(symbols),
             immutabledict(relocation_symbols),
-            bss_size_required,
         )
 
     @staticmethod
@@ -269,15 +262,13 @@ def make_bom(
             object_map.update(r)
 
         # Compute the required size for the .bss segment
-        bss_size_required, unresolved_sym_set = self._resolve_symbols_within_BOM(
-            object_map, entry_point_name
-        )
+        unresolved_sym_set = self._resolve_symbols_within_BOM(object_map, entry_point_name)
 
         return BOM(
             name,
             immutabledict(object_map),
             unresolved_sym_set,
-            bss_size_required,
+            None,
             entry_point_name,
             self._toolchain.segment_alignment,
         )
@@ -303,12 +294,10 @@ def _create_object_file(
 
     def _resolve_symbols_within_BOM(
         self, object_map: Dict[str, AssembledObject], entry_point_name: Optional[str] = None
-    ) -> Tuple[int, Set[str]]:
-        bss_size_required = 0
+    ) -> Set[str]:
         symbols: Dict[str, Tuple[int, LinkableSymbolType]] = {}
         unresolved_symbols: Dict[str, Tuple[int, LinkableSymbolType]] = {}
         for o in object_map.values():
-            bss_size_required += o.bss_size_required
             symbols.update(o.strong_symbols)
             # Resolve symbols defined within different patch files within the same patch BOM
             for sym, values in o.unresolved_symbols.items():
@@ -322,7 +311,7 @@ def _resolve_symbols_within_BOM(
         if entry_point_name and entry_point_name not in symbols:
             raise PatchMakerException(f"Entry point {entry_point_name} not found in object files")
 
-        return bss_size_required, unresolved_sym_set
+        return unresolved_sym_set
 
     def create_unsafe_bss_segment(self, vm_address: int, size: int) -> Segment:
         """
@@ -336,13 +325,21 @@ def create_unsafe_bss_segment(self, vm_address: int, size: int) -> Segment:
 
         :return: a [Segment][ofrak_patch_maker.toolchain.model.Segment] object.
         """
+
+        warn(
+            "Reserving .bss space at the FEM linking step is deprecated! Run "
+            "FreeSpaceModifier with data_range=None before allocating the BOM instead.",
+            category=DeprecationWarning,
+        )
+
         segment = Segment(
             segment_name=".bss",
             vm_address=vm_address,
             offset=0x0,
             is_entry=False,
             length=size,
             access_perms=MemoryPermissions.RW,
+            is_bss=True,
         )
         align = self._toolchain.segment_alignment
         if vm_address % align != 0:
@@ -375,7 +372,7 @@ def _build_ld(
         architecture-specific choices here.
 
         :param boms: BOMs and their corresponding target memory descriptions
-        :param bss_segment: A `.bss` segment, if any
+        :param bss_segment: A `.bss` segment, if any. DEPRECATED.
         :param additional_symbols: Additional symbols to provide to this patch, if needed
 
         :return: path to `.ld` script file
@@ -389,8 +386,28 @@ def _build_ld(
                 for segment in region_config.segments[obj.path]:
                     # Skip the segments we're not interested in.
                     # We have to create regions for 0-length segments to keep the linker happy!
-                    if not self._toolchain.keep_section(segment.segment_name):
+                    if not segment.is_allocated:
+                        continue
+
+                    if segment.vm_address == Segment.NOBITS_LEGACY_VADDR:
+                        if not segment.is_bss:
+                            raise ValueError(
+                                "NOBITS_LEGACY_VADDR only supported for legacy allocation of .bss segments"
+                            )
+
+                        warn(
+                            "Reserving .bss space at the FEM linking step is deprecated! Run "
+                            "FreeSpaceModifier with data_range=None before allocating the BOM instead.",
+                            category=DeprecationWarning,
+                        )
+
+                        if not bss_segment:
+                            raise PatchMakerException(
+                                f"BOM {bom.name} requires bss but no bss Segment allocation provided"
+                            )
+                        bss_size_required += segment.length
                         continue
+
                     memory_region, memory_region_name = self._toolchain.ld_generate_region(
                         obj.path,
                         segment.segment_name,
@@ -400,17 +417,10 @@ def _build_ld(
                     )
                     memory_regions.append(memory_region)
                     section = self._toolchain.ld_generate_section(
-                        obj.path, segment.segment_name, memory_region_name
+                        obj.path, segment.segment_name, memory_region_name, segment.is_bss
                     )
                     sections.append(section)
 
-            if bom.bss_size_required > 0:
-                if not bss_segment:
-                    raise PatchMakerException(
-                        f"BOM {bom.name} requires bss but no bss Segment allocation provided"
-                    )
-                bss_size_required += bom.bss_size_required
-
             if self._toolchain.is_relocatable():
                 (
                     reloc_regions,
@@ -470,7 +480,7 @@ def make_fem(
 
         :param boms:
         :param exec_path:
-        :param unsafe_bss_segment:
+        :param unsafe_bss_segment: DEPRECATED
         :param additional_symbols:
 
         :raises PatchMakerException: for invalid user inputs