[PATCH v9 09/19] bfd: parse Object Attributes v2's section in input object files
Jan Beulich
jbeulich@suse.com
Fri Oct 31 10:25:14 GMT 2025
On 01.09.2025 18:56, Matthieu Longo wrote:
> This patch adds to BFD the capability of parsing the Object Attributes v2
> (OAv2) stored into the section defined by the backend (i.e. .ARM.attributes
> for AArch64).
>
> An OAv2 section is organized with the following syntactic structure:
> <format-version: uint8>
> [ <subsection-length: uint32>
> <subsection-name: NTBS>
> <optionality: uint8>
> <encoding: uint8>
> <attribute>*
> ]*
>
> Once parsing is completed, OAv2 are stored in a list of subsections in
> elf_obj_tdata.
>
> Regarding ULEB128 parsing, the implementation of _bfd_safe_read_leb128
> has known shortcomings. In particular, it may accept malformed ULEB128
> values if the 8th bit is set while meeting the end condition "data < end".
> The error handling could be improved, but this function is old and used
> in more than 50 places. The same issue applies to _bfd_read_unsigned_leb128
> and _bfd_read_signed_leb128, which likewise do not report errors in
> this case. Since this is long-standing behavior and fixing it would
> require broader changes outside the scope of this patch series, the
> issue is acknowledged but not addressed here, and is considered
> acceptable as this patch does not worsen the situation.
> ---
> bfd/elf-attrs.c | 254 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 251 insertions(+), 3 deletions(-)
The way the helper functions are (naturally) organized, I decided to look at
this bottom-up (but of course top-down within each function). Reading the
comments in this order may therefore make a little more sense here and there.
> --- a/bfd/elf-attrs.c
> +++ b/bfd/elf-attrs.c
> @@ -843,7 +843,7 @@ _bfd_elf_obj_attrs_arg_type (bfd *abfd,
> }
>
> static void
> -bfd_elf_parse_attr_section_v1 (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
> +oav1_parse_section (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
> {
> const char *std_sec = get_elf_backend_data (abfd)->obj_attrs_vendor;
>
> @@ -962,7 +962,245 @@ bfd_elf_parse_attr_section_v1 (bfd *abfd, bfd_byte *p, bfd_byte *p_end)
> }
> }
>
> -/* Parse an object attributes section. */
> +#define READ_ULEB128(abfd, var, cursor, end, total_read) \
> + do \
> + { \
> + bfd_byte *begin = (cursor); \
> + (var) = _bfd_safe_read_leb128 (abfd, &(cursor), false, end); \
> + (total_read) += (cursor) - begin; \
> + } \
> + while (0)
> +
> +static int
> +read_ntbs (bfd *abfd,
> + const bfd_byte *cursor,
> + const bfd_byte *end,
> + const char **s)
> +{
> + *s = NULL;
> + const size_t MAX_STR_LEN = end - cursor; /* Including \0. */
> + const size_t s_len = strnlen ((char *) cursor, MAX_STR_LEN);
Please don't cast away const-ness unless you really need to. See ...
> + if (s_len == MAX_STR_LEN)
> + {
> + bfd_set_error (bfd_error_malformed_archive);
> + _bfd_error_handler (_("%pB: error: NTBS value seems corrupted "
> + "(missing '\\0')"), abfd);
> + return -1;
> + }
> + *s = xstrdup ((const char *) cursor);
... e.g. here.
This latter cast could actually be avoided: As you know the length, you could
easily use xmemdup() instead. That'll also be cheaper, as the length won't
need determining a 2nd time then.
> + return s_len + 1;
> +}
> +
> +/* Parse an object attribute (v2 only). */
> +static int
> +oav2_parse_attr (bfd *abfd,
> + bfd_byte *cursor,
> + const bfd_byte *end,
> + obj_attr_encoding_v2 attr_type,
> + obj_attr_v2 **attr)
> +{
> + *attr = NULL;
> + size_t total_read = 0;
> +
> + obj_attr_tag_t attr_tag;
> + READ_ULEB128 (abfd, attr_tag, cursor, end, total_read);
> +
> + union obj_attr_value_v2 attr_val;
> + switch (attr_type)
> + {
> + case OA_ENC_NTBS:
> + {
> + int read = read_ntbs (abfd, cursor, end, &attr_val.string_val);
> + if (read <= 0)
> + return -1;
> + total_read += read;
> + }
> + break;
> + case OA_ENC_ULEB128:
> + READ_ULEB128 (abfd, attr_val.uint_val, cursor, end, total_read);
> + break;
> + default:
> + abort ();
> + }
> +
> + *attr = _bfd_elf_obj_attr_v2_init (attr_tag, attr_val);
> + return total_read;
> +}
> +
> +/* Parse a subsection (object attributes v2 only). */
> +static int
> +oav2_parse_subsection (bfd *abfd,
> + bfd_byte *cursor,
> + const uint64_t max_read,
> + obj_attr_subsection_v2 **subsec)
> +{
> + *subsec = NULL;
> + size_t total_read = 0;
> +
> + const char *subsection_name = NULL;
I'm all for pointer-to-const where it's helpful. Here it doesn't look to be; its
main (bad) effect is the need to cast away the const when calling free(). For the
purpose of the function it also doesn't matter - it is here where the string is
read (and hence being modified anyway).
> + const uint32_t F_SUBSECTION_LEN = sizeof(uint32_t);
> + const uint32_t F_SUBSECTION_COMPREHENSION = sizeof(uint8_t);
> + const uint32_t F_SUBSECTION_ENCODING = sizeof(uint8_t);
> + /* The minimum subsection length is 7: 4 bytes for the length itself, and 1
> + byte for an empty NUL-terminated string, 1 byte for the comprehension,
> + 1 byte for the encoding, and no vendor-data. */
> + const uint32_t F_MIN_SUBSECTION_DATA_LEN
> + = F_SUBSECTION_LEN + 1 /* for '\0' */
> + + F_SUBSECTION_COMPREHENSION + F_SUBSECTION_ENCODING;
> +
> + /* Similar to the issues reported in PR 17531, we need to check all the sizes
> + and offsets as we parse the section. */
> + if (max_read < F_MIN_SUBSECTION_DATA_LEN)
> + {
> + _bfd_error_handler (_("%pB: error: attributes subsection ends "
> + "prematurely"), abfd);
> + goto error;
> + }
> +
> + const uint32_t subsection_len = bfd_get_32 (abfd, cursor);
> + total_read += F_SUBSECTION_LEN;
> + cursor += F_SUBSECTION_LEN;
> + if (subsection_len > max_read)
> + {
> + _bfd_error_handler (_("%pB: error: bad subsection length (%u > max=%lu)"),
> + abfd, subsection_len, max_read);
> + goto error;
> + }
> + else if (subsection_len < F_MIN_SUBSECTION_DATA_LEN)
> + {
> + _bfd_error_handler (_("%pB: error: subsection length of %u is too small"),
> + abfd, subsection_len);
> + goto error;
> + }
> +
> + const size_t MAX_SUBSECTION_NAME_LEN
> + = subsection_len - F_SUBSECTION_LEN
> + - F_SUBSECTION_COMPREHENSION - F_SUBSECTION_ENCODING;
Other all-capitals identifiers are kind-of #define-s of build-time constants.
This one isn't a constant, though, so the use of such an identifier feels a
little misleading.
> + const size_t subsection_name_len
> + = strnlen ((char *) cursor, MAX_SUBSECTION_NAME_LEN);
I think I mentioned earlier that casts would better be used sparingly. Here we
could get away without, if we used memchr() instead. Thoughts?
> + if (subsection_name_len >= MAX_SUBSECTION_NAME_LEN)
> + {
> + _bfd_error_handler (_("%pB: error: subsection name seems corrupted "
> + "(missing '\\0')"), abfd);
> + goto error;
> + }
> + /* Note: if the length of the subsection name is 0 (i.e. the string is '\0'),
> + it is still considered a valid name, even if it is not particularly
> + useful. */
> +
> + const bfd_byte *const end = cursor - F_SUBSECTION_LEN + subsection_len;
The expression could be simplified if this was moved up a little, and the
earlier update of cursor also moved down some.
> + /* Note: at this stage,
> + 1. the length of the subsection name is validated, as the presence of '\0'
> + at the end of the string, so no risk of buffer overrun.
> + 2. the data for comprehension and encoding can also safely be read. */
> + {
> + int read = read_ntbs (abfd, cursor, cursor + subsection_name_len + 1,
> + &subsection_name);
> + total_read += read;
> + cursor += read;
> + }
> +
> + uint8_t comprehension_raw = bfd_get_8 (abfd, cursor);
> + ++cursor;
> + ++total_read;
> +
> + /* Comprehension is supposed to be a boolean, so any value greater than 1 is
> + considered invalid. */
> + if (comprehension_raw > 1)
> + {
> + _bfd_error_handler (_("%pB: error: '%s' seems corrupted, got %u but only "
> + "0x0 ('%s') or 0x1 ('%s') are valid values."),
Are the 0x here of any real use?
Also, throughout the series (as this looks to be repeating a lot, albeit you're
also not consistent there): Diagnostics should normally not end in a full stop.
> + abfd, "comprehension", comprehension_raw,
> + "required", "optional");
> + goto error;
> + }
> +
> + uint8_t value_encoding_raw = bfd_get_8 (abfd, cursor);
> + ++cursor;
> + ++total_read;
> +
> + /* Encoding cannot be greater than OA_ENC_MAX, otherwise it means that either
> + there is a new encoding that was introduced in the spec, and this
> + implementation in binutils is older, and not aware of it so does not
> + support it; or the stored value for encoding is garbage. */
> + enum obj_attr_encoding_v2 value_encoding
> + = obj_attr_encoding_v2_from_u8 (value_encoding_raw);
> + if (value_encoding > OA_ENC_MAX)
> + {
> + _bfd_error_handler (_("%pB: error: attribute type seems corrupted, got"
> + " %u but only 0x0 (ULEB128) or 0x1 (NTBS) are "
> + "valid types."), abfd, value_encoding);
Same remarks here.
> + goto error;
> + }
> +
> + const char *vendor_name = get_elf_backend_data (abfd)->obj_attrs_vendor;
> + obj_attr_subsection_scope_v2 scope
> + = (strncmp (subsection_name, vendor_name, strlen (vendor_name)) == 0
> + ? OA_SUBSEC_PUBLIC
> + : OA_SUBSEC_PRIVATE);
What is this following from? I'm in particular puzzled that there's no form of
separator required between the part potentially matching the sub-section name
and the rest. This way a sub-section XYZ would be public for vendor XY, which
may or may not be what is intended. Personally I would have expected
separation by e.g. some non-alphanumeric character.
> + *subsec = _bfd_elf_obj_attr_subsection_v2_init
> + (subsection_name, scope, comprehension_raw, value_encoding);
> +
> + /* A subsection can be empty, so 'cursor' can be equal to 'end' here. */
> + bool err = false;
> + while (!err && cursor < end)
> + {
> + obj_attr_v2 *attr;
> + int read = oav2_parse_attr (abfd, cursor, end, value_encoding, &attr);
> + LINKED_LIST_APPEND(obj_attr_v2) (*subsec, attr);
If you got back an error, you have nothing to append (you'd append NULL, which
I don't expect will be liked by the list machinery).
> + total_read += read;
> + err |= (read < 0);
> + cursor += read;
> + }
> +
> + if (err)
> + {
> + _bfd_elf_obj_attr_subsection_v2_free (*subsec);
> + *subsec = NULL;
> + return -1;
> + }
> +
> + BFD_ASSERT (cursor == end);
> + return total_read;
> +
> + error:
> + bfd_set_error (bfd_error_malformed_archive);
> + if (subsection_name)
> + free ((void *) subsection_name);
> + return -1;
> +}
> +
> +/* Parse the list of subsections (object attributes v2 only). */
> +static void
> +oav2_parse_section (bfd *abfd,
> + Elf_Internal_Shdr *hdr,
> + bfd_byte *cursor)
As previously requested: Pointer-to-const please wherever possible. At least
the latter two parameters look to qualify. Hmm, or maybe not for cursor, due
to _bfd_safe_read_leb128() wanting it non-const.
> +{
> + obj_attr_subsection_list *subsecs = &elf_obj_attr_subsections (abfd);
> + int read = 0;
> + for (uint64_t remaining = hdr->sh_size - 1; /* Already read 'A'. */
> + remaining > 0;
> + remaining -= read, cursor += read)
> + {
> + obj_attr_subsection_v2 *subsec = NULL;
> + read = oav2_parse_subsection (abfd, cursor, remaining, &subsec);
> + if (read < 0)
"read" being plain int, it looks to be implied that a subsection can't
exceed 2Gb in size? (There's no explicit check anywhere that I could spot.
And yes, such a big section might indeed be insane, but I assume you know
what e.g. fuzzers do.)
> + {
> + _bfd_error_handler (_("%pB: error: could not parse subsection at "
> + "offset %lx"),
> + abfd, hdr->sh_size - remaining);
As also previously indicated: You can't use %lx with uint64_t-type variables.
It needs to remain possible to build binutils on 32-bit hosts.
Jan
More information about the Binutils
mailing list