[v2] libebl: recognize FDO Packaging Metadata ELF note

  From: Luca Boccassi <bluca@debian.org>

As defined on: https://systemd.io/COREDUMP_PACKAGE_METADATA/
this note will be used starting from Fedora 36. Allow
readelf --notes to pretty print it:

Note section [ 3] '.note.package' of 76 bytes at offset 0x2e8:
  Owner          Data size  Type
  FDO                   57  FDO_PACKAGING_METADATA
    Packaging Metadata: {"type":"deb","name":"fsverity-utils","version":"1.3-1"}

Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
---
v2: check that note payload is NULL terminated

 libebl/eblobjnote.c         | 3 +++
 libebl/eblobjnotetypename.c | 3 +++
 libelf/elf.h                | 3 +++
 3 files changed, 9 insertions(+)
  

On Sun, 2021-11-21 at 19:43 +0000, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> As defined on: https://systemd.io/COREDUMP_PACKAGE_METADATA/
> this note will be used starting from Fedora 36. Allow
> readelf --notes to pretty print it:
> 
> Note section [ 3] '.note.package' of 76 bytes at offset 0x2e8:
>   Owner          Data size  Type
>   FDO                   57  FDO_PACKAGING_METADATA
>     Packaging Metadata: {"type":"deb","name":"fsverity-utils","version":"1.3-1"}
> 
> Signed-off-by: Luca Boccassi <luca.boccassi@microsoft.com>
> ---
> v2: check that note payload is NULL terminated
> 
>  libebl/eblobjnote.c         | 3 +++
>  libebl/eblobjnotetypename.c | 3 +++
>  libelf/elf.h                | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/libebl/eblobjnote.c b/libebl/eblobjnote.c
> index 36efe275..e3ad1409 100644
> --- a/libebl/eblobjnote.c
> +++ b/libebl/eblobjnote.c
> @@ -288,6 +288,9 @@ ebl_object_note (Ebl *ebl, uint32_t namesz, const char *name, uint32_t type,
>        if (descsz == 0 && type == NT_VERSION)
>  	return;
>  
> 
> 
> 
> +      if (strcmp ("FDO", name) == 0 && type == FDO_PACKAGING_METADATA && descsz > 0 && desc[descsz - 1] == '\0')
> +	printf("    Packaging Metadata: %.*s\n", (int) descsz, desc);
> +
>        /* Everything else should have the "GNU" owner name.  */
>        if (strcmp ("GNU", name) != 0)
>  	return;
> diff --git a/libebl/eblobjnotetypename.c b/libebl/eblobjnotetypename.c
> index 4662906d..863f20e4 100644
> --- a/libebl/eblobjnotetypename.c
> +++ b/libebl/eblobjnotetypename.c
> @@ -101,6 +101,9 @@ ebl_object_note_type_name (Ebl *ebl, const char *name, uint32_t type,
>  	  return buf;
>  	}
>  
> 
> 
> 
> +      if (strcmp (name, "FDO") == 0 && type == FDO_PACKAGING_METADATA)
> +	return "FDO_PACKAGING_METADATA";
> +
>        if (strcmp (name, "GNU") != 0)
>  	{
>  	  /* NT_VERSION is special, all data is in the name.  */
> diff --git a/libelf/elf.h b/libelf/elf.h
> index 8e3e618f..633f9f67 100644
> --- a/libelf/elf.h
> +++ b/libelf/elf.h
> @@ -1297,6 +1297,9 @@ typedef struct
>  /* Program property.  */
>  #define NT_GNU_PROPERTY_TYPE_0 5
>  
> 
> 
> 
> +/* Packaging metadata as defined on https://systemd.io/COREDUMP_PACKAGE_METADATA/ */
> +#define FDO_PACKAGING_METADATA 0xcafe1a7e
> +
>  /* Note section name of program property.   */
>  #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property"

Hi Mark,

I could move this definition to an internal header if the change to
elf.h blocks this patch, if you prefer? Let me know.
  

Hi Luca,

On Thu, 2021-11-25 at 17:02 +0000, Luca Boccassi via Elfutils-devel
wrote:
> +/* Packaging metadata as defined on 
> > https://systemd.io/COREDUMP_PACKAGE_METADATA/ */
> > +#define FDO_PACKAGING_METADATA 0xcafe1a7e
> > +
> >  /* Note section name of program property.   */
> >  #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property"
> 
> I could move this definition to an internal header if the change to
> elf.h blocks this patch, if you prefer? Let me know.

It looks like it will be integrated into glibc elf.h later this week.
I'll resync elf.h then and apply the other half of your patch.

While looking at how to implement the json parsing of the note data I
noticed a couple of things that could be clarified in the spec to make
this more clear and less ambiguous.

The spec says "a key-value JSON format", "JSON payload" and "a JSON
string with the structure described below". Which isn't very exact, or
seems to describe multiple different JSON concepts which aren't exactly
the same thing. A JSON string is something different from a JSON object
(which is the only JSON value that has a key-value format). And it
doesn't really make clear what the encoding of the JSON itself is (it
cannot be a JSON string, because that itself is expressed in an
specific character encoding itself).

What I think the spec should say is something like:
"The note data is a single JSON object encoded as a zero terminated
UTF-8 string."

The spec does explain the requirements for JSON numbers, but doesn't
mention any for JSON strings or JSON objects. It would be good to also
explain how to make the strings and objects unambiguous.

For Objects it should require that all names are unique. See section 4
in rfc8259.

For Strings it should require that \uXXXX escaping isn't used and that
only control characters that have an explicit escape sequence are used
(\b, \n, \f, \r, \t) [if you allow them in the first place, they are
probably confusing and it may be good to simply say that Strings should
not contain any control characters or use \uXXXX escaping]. See section
7 and section 8 in rfc8259.

That should get rid of various corner cases that different parsers are
known to get wrong. Especially \uXXXX escaping is really confusing when
using the UTF-8 encoding (and it is totally necessary since UTF-8 can
express any valid UTF character already).

Cheers,

Mark
  

On Tue, 2021-11-30 at 12:25 +0100, Mark Wielaard wrote:
> Hi Luca,
> 
> On Thu, 2021-11-25 at 17:02 +0000, Luca Boccassi via Elfutils-devel
> wrote:
> > +/* Packaging metadata as defined on 
> > > https://systemd.io/COREDUMP_PACKAGE_METADATA/ */
> > > +#define FDO_PACKAGING_METADATA 0xcafe1a7e
> > > +
> > >  /* Note section name of program property.   */
> > >  #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property"
> > 
> > I could move this definition to an internal header if the change to
> > elf.h blocks this patch, if you prefer? Let me know.
> 
> It looks like it will be integrated into glibc elf.h later this week.
> I'll resync elf.h then and apply the other half of your patch.
> 
> While looking at how to implement the json parsing of the note data I
> noticed a couple of things that could be clarified in the spec to make
> this more clear and less ambiguous.
> 
> The spec says "a key-value JSON format", "JSON payload" and "a JSON
> string with the structure described below". Which isn't very exact, or
> seems to describe multiple different JSON concepts which aren't exactly
> the same thing. A JSON string is something different from a JSON object
> (which is the only JSON value that has a key-value format). And it
> doesn't really make clear what the encoding of the JSON itself is (it
> cannot be a JSON string, because that itself is expressed in an
> specific character encoding itself).
> 
> What I think the spec should say is something like:
> "The note data is a single JSON object encoded as a zero terminated
> UTF-8 string."
> 
> The spec does explain the requirements for JSON numbers, but doesn't
> mention any for JSON strings or JSON objects. It would be good to also
> explain how to make the strings and objects unambiguous.
> 
> For Objects it should require that all names are unique. See section 4
> in rfc8259.
> 
> For Strings it should require that \uXXXX escaping isn't used and that
> only control characters that have an explicit escape sequence are used
> (\b, \n, \f, \r, \t) [if you allow them in the first place, they are
> probably confusing and it may be good to simply say that Strings should
> not contain any control characters or use \uXXXX escaping]. See section
> 7 and section 8 in rfc8259.
> 
> That should get rid of various corner cases that different parsers are
> known to get wrong. Especially \uXXXX escaping is really confusing when
> using the UTF-8 encoding (and it is totally necessary since UTF-8 can
> express any valid UTF character already).
> 
> Cheers,
> 
> Mark

Thanks, see:

https://github.com/systemd/systemd/pull/21578
  

Hi -

On Tue, Nov 30, 2021 at 12:25:41PM +0100, Mark Wielaard wrote:
> [...]
> The spec does explain the requirements for JSON numbers, but doesn't
> mention any for JSON strings or JSON objects. It would be good to also
> explain how to make the strings and objects unambiguous. [...]
> For Objects it should require that all names are unique. [...]
> For Strings it should require that \uXXXX escaping isn't used [...]
> 
> That should get rid of various corner cases that different parsers are
> known to get wrong. 

Are such buggy parsers seen in the wild, now, after all this time with
JSON?  It seems to me it's not really elfutils' or systemd's place to
impose -additional- constraints on customary JSON.


> Especially \uXXXX escaping is really confusing when using the UTF-8
> encoding (and it is totally necessary since UTF-8 can express any
> valid UTF character already).

Yes, and yet we have had the bidi situation recently where UTF-8 raw
codes could visually confuse a human reader whereas escaped \uXXXX
wouldn't.  If we forbid \uXXXX unilaterally, we literally become
incompatible with JSON (RFC8259 7. String. "Any character may be
escaped."), and for what?

Both these seem to be personal aesthetic decisions that need not be
imposed on a routine application of well-established standards.  We
have many industrial-strength json parser libraries to choose from,
so that part is IMO no longer a timely concern.


- FChE
  

* Frank Ch. Eigler via Elfutils-devel:

> Hi -
>
> On Tue, Nov 30, 2021 at 12:25:41PM +0100, Mark Wielaard wrote:
>> [...]
>> The spec does explain the requirements for JSON numbers, but doesn't
>> mention any for JSON strings or JSON objects. It would be good to also
>> explain how to make the strings and objects unambiguous. [...]
>> For Objects it should require that all names are unique. [...]
>> For Strings it should require that \uXXXX escaping isn't used [...]
>> 
>> That should get rid of various corner cases that different parsers are
>> known to get wrong. 
>
> Are such buggy parsers seen in the wild, now, after all this time with
> JSON?  It seems to me it's not really elfutils' or systemd's place to
> impose -additional- constraints on customary JSON.

JSON has been targeted at the Windows/Java UTF-16 world, there is always
going to be a mismatch if you try to represent it in UTF-8 or anything
that doesn't have surrogate pairs.

>> Especially \uXXXX escaping is really confusing when using the UTF-8
>> encoding (and it is totally necessary since UTF-8 can express any
>> valid UTF character already).
>
> Yes, and yet we have had the bidi situation recently where UTF-8 raw
> codes could visually confuse a human reader whereas escaped \uXXXX
> wouldn't.  If we forbid \uXXXX unilaterally, we literally become
> incompatible with JSON (RFC8259 7. String. "Any character may be
> escaped."), and for what?

RFC 8259 says this:

   However, the ABNF in this specification allows member names and
   string values to contain bit sequences that cannot encode Unicode
   characters; for example, "\uDEAD" (a single unpaired UTF-16
   surrogate).  Instances of this have been observed, for example, when
   a library truncates a UTF-16 string without checking whether the
   truncation split a surrogate pair.  The behavior of software that
   receives JSON texts containing such values is unpredictable; for
   example, implementations might return different values for the length
   of a string value or even suffer fatal runtime exceptions.

A UTF-8 environment has to enforce *some* additional constraints
compared to the official JSON syntax.

Thanks,
Florian
  

Hi,

On Tue, Nov 30, 2021 at 05:49:58PM +0100, Florian Weimer wrote:
> * Frank Ch. Eigler via Elfutils-devel:
> > On Tue, Nov 30, 2021 at 12:25:41PM +0100, Mark Wielaard wrote:
> >> [...]
> >> The spec does explain the requirements for JSON numbers, but doesn't
> >> mention any for JSON strings or JSON objects. It would be good to also
> >> explain how to make the strings and objects unambiguous. [...]
> >> For Objects it should require that all names are unique. [...]
> >> For Strings it should require that \uXXXX escaping isn't used [...]
> >> 
> >> That should get rid of various corner cases that different parsers are
> >> known to get wrong. 
> >
> > Are such buggy parsers seen in the wild, now, after all this time with
> > JSON?  It seems to me it's not really elfutils' or systemd's place to
> > impose -additional- constraints on customary JSON.
> 
> JSON has been targeted at the Windows/Java UTF-16 world, there is always
> going to be a mismatch if you try to represent it in UTF-8 or anything
> that doesn't have surrogate pairs.

Right, the \uhex escaping is kind of odd when using UTF-8 because you
suddenly have to use surrogate pairs. It is correct that you may use
them according to the RFC. But the RFC also points out that if you do
there are various odd/bad corner cases to take care of and that it
makes comparing strings really awkward.

> >> Especially \uXXXX escaping is really confusing when using the UTF-8
> >> encoding (and it is totally necessary since UTF-8 can express any
> >> valid UTF character already).
> >
> > Yes, and yet we have had the bidi situation recently where UTF-8 raw
> > codes could visually confuse a human reader whereas escaped \uXXXX
> > wouldn't.  If we forbid \uXXXX unilaterally, we literally become
> > incompatible with JSON (RFC8259 7. String. "Any character may be
> > escaped."), and for what?
> 
> RFC 8259 says this:
> 
>    However, the ABNF in this specification allows member names and
>    string values to contain bit sequences that cannot encode Unicode
>    characters; for example, "\uDEAD" (a single unpaired UTF-16
>    surrogate).  Instances of this have been observed, for example, when
>    a library truncates a UTF-16 string without checking whether the
>    truncation split a surrogate pair.  The behavior of software that
>    receives JSON texts containing such values is unpredictable; for
>    example, implementations might return different values for the length
>    of a string value or even suffer fatal runtime exceptions.
> 
> A UTF-8 environment has to enforce *some* additional constraints
> compared to the official JSON syntax.

Right, all recommendations for encoding the JSON object, string and
numbers actually come from the RFC. I don't think it is odd to
explicitly specify the encoding of the package note as using a zero
terminated UTF-8 string and to specify the allowed JSON string and
number encodings. The contents of the package note is meant to be
human readable, so excluding control characters and excotic escape
sequence in the expected key/value pairs seems natural to me.

Cheers,

Mark
  

Hi -

> JSON has been targeted at the Windows/Java UTF-16 world, there is always
> going to be a mismatch if you try to represent it in UTF-8 or anything
> that doesn't have surrogate pairs.

The JSON RFC8259 8.1 mandates UTF-8 encoding for situations like ours.


> > Yes, and yet we have had the bidi situation recently where UTF-8 raw
> > codes could visually confuse a human reader whereas escaped \uXXXX
> > wouldn't.  If we forbid \uXXXX unilaterally, we literally become
> > incompatible with JSON (RFC8259 7. String. "Any character may be
> > escaped."), and for what?
> 
> RFC 8259 says this:
> 
>    However, the ABNF in this specification allows member names and
>    string values to contain bit sequences that cannot encode Unicode
>    characters; for example, "\uDEAD" (a single unpaired UTF-16
>    surrogate).  Instances of this have been observed, for example, when
>    a library truncates a UTF-16 string without checking whether the
>    truncation split a surrogate pair.  The behavior of software that
>    receives JSON texts containing such values is unpredictable; for
>    example, implementations might return different values for the length
>    of a string value or even suffer fatal runtime exceptions.
> 
> A UTF-8 environment has to enforce *some* additional constraints
> compared to the official JSON syntax.

I'm sorry, I don't see how.  If a JSON string were to include the
suspect "\uDEAD", but from observing our hypothetical "no escapes!"
rule they could reencode it as the UTF-8 octets 0xED 0xBA 0xAD.
ISTM we're no better off.


- FChE
  

* Frank Ch. Eigler:

>> > Yes, and yet we have had the bidi situation recently where UTF-8 raw
>> > codes could visually confuse a human reader whereas escaped \uXXXX
>> > wouldn't.  If we forbid \uXXXX unilaterally, we literally become
>> > incompatible with JSON (RFC8259 7. String. "Any character may be
>> > escaped."), and for what?
>> 
>> RFC 8259 says this:
>> 
>>    However, the ABNF in this specification allows member names and
>>    string values to contain bit sequences that cannot encode Unicode
>>    characters; for example, "\uDEAD" (a single unpaired UTF-16
>>    surrogate).  Instances of this have been observed, for example, when
>>    a library truncates a UTF-16 string without checking whether the
>>    truncation split a surrogate pair.  The behavior of software that
>>    receives JSON texts containing such values is unpredictable; for
>>    example, implementations might return different values for the length
>>    of a string value or even suffer fatal runtime exceptions.
>> 
>> A UTF-8 environment has to enforce *some* additional constraints
>> compared to the official JSON syntax.
>
> I'm sorry, I don't see how.  If a JSON string were to include the
> suspect "\uDEAD", but from observing our hypothetical "no escapes!"
> rule they could reencode it as the UTF-8 octets 0xED 0xBA 0xAD.
> ISTM we're no better off.

These octets aren't UTF-8.  UTF-8 never contains surrogate pairs (paired
or unpaired). 8-(

Thanks,
Florian
  

Hi Luca,

On Tue, Nov 30, 2021 at 12:25:41PM +0100, Mark Wielaard wrote:
> On Thu, 2021-11-25 at 17:02 +0000, Luca Boccassi via Elfutils-devel
> wrote:
> > +/* Packaging metadata as defined on 
> > > https://systemd.io/COREDUMP_PACKAGE_METADATA/ */
> > > +#define FDO_PACKAGING_METADATA 0xcafe1a7e
> > > +
> > >  /* Note section name of program property.   */
> > >  #define NOTE_GNU_PROPERTY_SECTION_NAME ".note.gnu.property"
> > 
> > I could move this definition to an internal header if the change to
> > elf.h blocks this patch, if you prefer? Let me know.
> 
> It looks like it will be integrated into glibc elf.h later this week.
> I'll resync elf.h then and apply the other half of your patch.

I haven't forgotten about this. The glibc elf.h change has been
integrated now. But when I wanted to resync with the elfutils
libelf/elf.h version I noticed something that look like ABI breakage:
https://sourceware.org/pipermail/libc-alpha/2021-December/133589.html

I am trying to get a response to that before syncing and integrating
your patch.

Sorry for the delay,

Mark
  

> I haven't forgotten about this. The glibc elf.h change has been
> integrated now. But when I wanted to resync with the elfutils
> libelf/elf.h version I noticed something that look like ABI
> breakage:
> https://sourceware.org/pipermail/libc-alpha/2021-December/133589.html
>
> I am trying to get a response to that before syncing and integrating
> your patch.

Sorry, I didn't like the answer I got. Basically this is ABI breakage,
it is just that the old constants were never really used, so some have
simply been renamed or given different constant values. Sigh.

That of course is not a good reason to then forget about your
patch. Apologies.

I took the elf.h update separately. Tweaked your patch a little and
added a patch of my own to make elflint recognize the new note type.

  [PATCH 1/3] libelf: Sync elf.h from glibc.
  [PATCH 2/3] libebl: recognize FDO Packaging Metadata ELF note
  [PATCH 3/3] elflint: Recognize NT_FDO_PACKAGING_METADATA

I saw Fedora 36 now has these new package notes. Sadly they omit the
debugInfoUrl field. Which makes them less useful imho. Do you happen
to know why that wasn't included?

Cheers,

Mark
  

On Fri, 2022-03-25 at 00:14 +0100, Mark Wielaard wrote:
> > I haven't forgotten about this. The glibc elf.h change has been
> > integrated now. But when I wanted to resync with the elfutils
> > libelf/elf.h version I noticed something that look like ABI
> > breakage:
> > https://sourceware.org/pipermail/libc-alpha/2021-December/133589.html
> > 
> > I am trying to get a response to that before syncing and
> > integrating
> > your patch.
> 
> Sorry, I didn't like the answer I got. Basically this is ABI
> breakage,
> it is just that the old constants were never really used, so some
> have
> simply been renamed or given different constant values. Sigh.
> 
> That of course is not a good reason to then forget about your
> patch. Apologies.
> 
> I took the elf.h update separately. Tweaked your patch a little and
> added a patch of my own to make elflint recognize the new note type.
> 
>   [PATCH 1/3] libelf: Sync elf.h from glibc.
>   [PATCH 2/3] libebl: recognize FDO Packaging Metadata ELF note
>   [PATCH 3/3] elflint: Recognize NT_FDO_PACKAGING_METADATA

No problem at all, change looks good, thanks for following up.

> I saw Fedora 36 now has these new package notes. Sadly they omit the
> debugInfoUrl field. Which makes them less useful imho. Do you happen
> to know why that wasn't included?

Not sure, I think the Fedora-side tooling was there already before we
added this to the spec, so it was simply not synced. I'll follow-up and
ask to get it included - might be too late for F36 unfortunately (not
sure new archive-wide rebuilds are going to happen), but if it gets
approved it should be there for F37.

I have included the field in the first PoC that uses the spec in
Debian, for the systemd packages:

$ readelf --notes /usr/lib/systemd/systemd | grep Packaging
    Packaging Metadata: {"type":"deb","os":"debian","name":"systemd","architecture":"amd64","version":"250.4-1","debugInfoUrl":"https://debuginfod.debian.net"}
  

Hi Luca,

On Fri, 2022-03-25 at 11:17 +0000, Luca Boccassi wrote:
> On Fri, 2022-03-25 at 00:14 +0100, Mark Wielaard wrote:
> > I took the elf.h update separately. Tweaked your patch a little and
> > added a patch of my own to make elflint recognize the new note
> > type.
> > 
> >   [PATCH 1/3] libelf: Sync elf.h from glibc.
> >   [PATCH 2/3] libebl: recognize FDO Packaging Metadata ELF note
> >   [PATCH 3/3] elflint: Recognize NT_FDO_PACKAGING_METADATA
> 
> No problem at all, change looks good, thanks for following up.

Thanks, I pushed these three patches.
But I noticed an issue on s390x fedora 36.
This isn't just elfutils though, binutils also has trouble:

Displaying notes found in: .note.package
  Owner                Data size        Description
readelf: /usr/bin/bash: Warning: note with invalid namesz and/or descsz
found at offset 0x0
readelf: /usr/bin/bash: Warning:  type: 0x7e1afeca, namesize:
0x04000000, descsize: 0x78000000, alignment: 4

Note how it seems the sizes are swapped. s390x is a big endian
platform.

Do you happen to know what/how the notes are created and if that
process might produce bad little/big encoding issues?

> I have included the field in the first PoC that uses the spec in
> Debian, for the systemd packages:
> 
> $ readelf --notes /usr/lib/systemd/systemd | grep Packaging
>     Packaging Metadata:
> {"type":"deb","os":"debian","name":"systemd","architecture":"amd64","
> version":"250.4-1","debugInfoUrl":"https://debuginfod.debian.net"}

Nice, thanks. I'll look into how to pick up the debugInfoUrl and use
that automagically if possible.

BTW. I notice that Fedora has an osCpe field where Debian has an os
field. It would imho be good if one or the other got standardized.

Cheers,

Mark
  

On Fri, 2022-03-25 at 14:39 +0100, Mark Wielaard wrote:
> Hi Luca,
> 
> On Fri, 2022-03-25 at 11:17 +0000, Luca Boccassi wrote:
> > On Fri, 2022-03-25 at 00:14 +0100, Mark Wielaard wrote:
> > > I took the elf.h update separately. Tweaked your patch a little and
> > > added a patch of my own to make elflint recognize the new note
> > > type.
> > > 
> > >   [PATCH 1/3] libelf: Sync elf.h from glibc.
> > >   [PATCH 2/3] libebl: recognize FDO Packaging Metadata ELF note
> > >   [PATCH 3/3] elflint: Recognize NT_FDO_PACKAGING_METADATA
> > 
> > No problem at all, change looks good, thanks for following up.
> 
> Thanks, I pushed these three patches.
> But I noticed an issue on s390x fedora 36.
> This isn't just elfutils though, binutils also has trouble:
> 
> Displaying notes found in: .note.package
>   Owner                Data size        Description
> readelf: /usr/bin/bash: Warning: note with invalid namesz and/or descsz
> found at offset 0x0
> readelf: /usr/bin/bash: Warning:  type: 0x7e1afeca, namesize:
> 0x04000000, descsize: 0x78000000, alignment: 4
> 
> Note how it seems the sizes are swapped. s390x is a big endian
> platform.
> 
> Do you happen to know what/how the notes are created and if that
> process might produce bad little/big encoding issues?

Agh - I knew big endianess was a bad idea! :-)
We have completely overlooked that, the note is created by a linker
script, which is generated at build time by this:

https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254

(well an older version in Fedora, but similar enough)

I'll flag this, thanks for the report

> > I have included the field in the first PoC that uses the spec in
> > Debian, for the systemd packages:
> > 
> > $ readelf --notes /usr/lib/systemd/systemd | grep Packaging
> >     Packaging Metadata:
> > {"type":"deb","os":"debian","name":"systemd","architecture":"amd64","
> > version":"250.4-1","debugInfoUrl":"https://debuginfod.debian.net"}
> 
> Nice, thanks. I'll look into how to pick up the debugInfoUrl and use
> that automagically if possible.
>
> BTW. I notice that Fedora has an osCpe field where Debian has an os
> field. It would imho be good if one or the other got standardized.

Debian has not adopted the CPE spec in its os-release, so there's no
way to 'source' the appropriate value. Also bear in mind the Debian
version is purely a PoC, opt-in and used only in the systemd package,
there's no proposal for distro-wide adoption. I plan to propose that
eventually, but it will not be anytime soon, other distros need to
adopt it first otherwise chances are it will just be rejected outright,
for no particular reason other than "it's a change, we don't like
change".
  

Hi Luca,

On Fri, 2022-03-25 at 13:52 +0000, Luca Boccassi wrote:
> On Fri, 2022-03-25 at 14:39 +0100, Mark Wielaard wrote:
> > But I noticed an issue on s390x fedora 36.
> > This isn't just elfutils though, binutils also has trouble:
> > 
> > Displaying notes found in: .note.package
> >   Owner                Data size        Description
> > readelf: /usr/bin/bash: Warning: note with invalid namesz and/or
> > descsz
> > found at offset 0x0
> > readelf: /usr/bin/bash: Warning:  type: 0x7e1afeca, namesize:
> > 0x04000000, descsize: 0x78000000, alignment: 4
> > 
> > Note how it seems the sizes are swapped. s390x is a big endian
> > platform.
> > 
> > Do you happen to know what/how the notes are created and if that
> > process might produce bad little/big encoding issues?
> 
> Agh - I knew big endianess was a bad idea! :-)
> We have completely overlooked that, the note is created by a linker
> script, which is generated at build time by this:
> 
> https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254
> 
> (well an older version in Fedora, but similar enough)

Yeah, that is definitely wrong. ELF is very careful about endianess. I
have a patch that detects it and works around it. But it is somewhat
ugly and has to work very low-level. So I am not sure I really want it.
Maybe I just apply it as a temporary workaround just for Fedora. But if
other distros have used such a script to generate package notes they
are also broken.

diff --git a/libelf/gelf_getnote.c b/libelf/gelf_getnote.c
index 0f7b9d68..6ef970c5 100644
--- a/libelf/gelf_getnote.c
+++ b/libelf/gelf_getnote.c
@@ -31,6 +31,7 @@
 #endif
 
 #include <assert.h>
+#include <byteswap.h>
 #include <gelf.h>
 #include <string.h>
 
@@ -73,6 +74,22 @@ gelf_getnote (Elf_Data *data, size_t offset, GElf_Nhdr *result,
 	offset = 0;
       else
 	{
+	  /* Workaround FDO package notes on big-endian systems,
+	     getting namesz and descsz wrong. Detect it by getting
+	     a bad namesz, descsz and byte swapped n_type for
+	     NT_FDO_PACKAGING_METADATA.  */
+	  if (unlikely (n->n_type == bswap_32 (NT_FDO_PACKAGING_METADATA)
+			&& n->n_namesz > data->d_size
+			&& n->n_descsz > data->d_size))
+	    {
+	      /* n might not be writable, use result and redirect n.  */
+	      *result = *n;
+	      result->n_type = bswap_32 (n->n_type);
+	      result->n_namesz = bswap_32 (n->n_namesz);
+	      result->n_descsz = bswap_32 (n->n_descsz);
+	      n = result;
+	    }
+
 	  /* This is slightly tricky, offset is guaranteed to be 4
 	     byte aligned, which is what we need for the name_offset.
 	     And normally desc_offset is also 4 byte aligned, but not

Note that Tom (on CC) submitted an IMHO much saner way to generate the
package notes using simple assembly which would have gotten all this
correct automagically.
https://src.fedoraproject.org/fork/tstellar/rpms/package-notes/c/25687ec2d8a4262d5ba5c55d35d68a994b892910

I see you rejected that, but please reconsider. Just hardcoding some
byte values really is broken.

Thanks,

Mark
  

On Fri, 2022-03-25 at 15:47 +0100, Mark Wielaard wrote:
> Hi Luca,
> 
> On Fri, 2022-03-25 at 13:52 +0000, Luca Boccassi wrote:
> > On Fri, 2022-03-25 at 14:39 +0100, Mark Wielaard wrote:
> > > But I noticed an issue on s390x fedora 36.
> > > This isn't just elfutils though, binutils also has trouble:
> > > 
> > > Displaying notes found in: .note.package
> > >   Owner                Data size        Description
> > > readelf: /usr/bin/bash: Warning: note with invalid namesz and/or
> > > descsz
> > > found at offset 0x0
> > > readelf: /usr/bin/bash: Warning:  type: 0x7e1afeca, namesize:
> > > 0x04000000, descsize: 0x78000000, alignment: 4
> > > 
> > > Note how it seems the sizes are swapped. s390x is a big endian
> > > platform.
> > > 
> > > Do you happen to know what/how the notes are created and if that
> > > process might produce bad little/big encoding issues?
> > 
> > Agh - I knew big endianess was a bad idea! :-)
> > We have completely overlooked that, the note is created by a linker
> > script, which is generated at build time by this:
> > 
> > https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254
> > 
> > (well an older version in Fedora, but similar enough)
> 
> Yeah, that is definitely wrong. ELF is very careful about endianess. I
> have a patch that detects it and works around it. But it is somewhat
> ugly and has to work very low-level. So I am not sure I really want it.
> Maybe I just apply it as a temporary workaround just for Fedora. But if
> other distros have used such a script to generate package notes they
> are also broken.
> 
> diff --git a/libelf/gelf_getnote.c b/libelf/gelf_getnote.c
> index 0f7b9d68..6ef970c5 100644
> --- a/libelf/gelf_getnote.c
> +++ b/libelf/gelf_getnote.c
> @@ -31,6 +31,7 @@
>  #endif
>  
> 
> 
> 
>  #include <assert.h>
> +#include <byteswap.h>
>  #include <gelf.h>
>  #include <string.h>
>  
> 
> 
> 
> @@ -73,6 +74,22 @@ gelf_getnote (Elf_Data *data, size_t offset, GElf_Nhdr *result,
>  	offset = 0;
>        else
>  	{
> +	  /* Workaround FDO package notes on big-endian systems,
> +	     getting namesz and descsz wrong. Detect it by getting
> +	     a bad namesz, descsz and byte swapped n_type for
> +	     NT_FDO_PACKAGING_METADATA.  */
> +	  if (unlikely (n->n_type == bswap_32 (NT_FDO_PACKAGING_METADATA)
> +			&& n->n_namesz > data->d_size
> +			&& n->n_descsz > data->d_size))
> +	    {
> +	      /* n might not be writable, use result and redirect n.  */
> +	      *result = *n;
> +	      result->n_type = bswap_32 (n->n_type);
> +	      result->n_namesz = bswap_32 (n->n_namesz);
> +	      result->n_descsz = bswap_32 (n->n_descsz);
> +	      n = result;
> +	    }
> +
>  	  /* This is slightly tricky, offset is guaranteed to be 4
>  	     byte aligned, which is what we need for the name_offset.
>  	     And normally desc_offset is also 4 byte aligned, but not

Thanks, but I don't think it's necessary to apply that just now - this
is a bug and I'll get a fix in the script first in the next couple
days, and then I'll chat with the Fedora dev working on this about what
to do regarding existing binaries.

> Note that Tom (on CC) submitted an IMHO much saner way to generate the
> package notes using simple assembly which would have gotten all this
> correct automagically.
> https://src.fedoraproject.org/fork/tstellar/rpms/package-notes/c/25687ec2d8a4262d5ba5c55d35d68a994b892910
> 
> I see you rejected that, but please reconsider. Just hardcoding some
> byte values really is broken.

The reality of having to deal with thirty thousand different build
system, integrated with different tools, and different packaging
systems, with different build scripts, on different distros, means that
ease of integration trumps over everything else. There are packages out
there using build systems that you couldn't even imagine in your worst
nightmares :-)
  

Hi Luca,

On Fri, Mar 25, 2022 at 02:55:14PM +0000, Luca Boccassi wrote:
> On Fri, 2022-03-25 at 15:47 +0100, Mark Wielaard wrote:
> > > We have completely overlooked that, the note is created by a linker
> > > script, which is generated at build time by this:
> > > 
> > > https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254
> > > 
> > > (well an older version in Fedora, but similar enough)
> > 
> > Yeah, that is definitely wrong. ELF is very careful about endianess. I
> > have a patch that detects it and works around it. But it is somewhat
> > ugly and has to work very low-level. So I am not sure I really want it.
> > Maybe I just apply it as a temporary workaround just for Fedora. But if
> > other distros have used such a script to generate package notes they
> > are also broken.
> > 
> > diff --git a/libelf/gelf_getnote.c b/libelf/gelf_getnote.c
> > [...]
> > @@ -73,6 +74,22 @@ gelf_getnote (Elf_Data *data, size_t offset, GElf_Nhdr *result,
> >  	offset = 0;
> >        else
> >  	{
> > +	  /* Workaround FDO package notes on big-endian systems,
> > +	     getting namesz and descsz wrong. Detect it by getting
> > +	     a bad namesz, descsz and byte swapped n_type for
> > +	     NT_FDO_PACKAGING_METADATA.  */
> > +	  if (unlikely (n->n_type == bswap_32 (NT_FDO_PACKAGING_METADATA)
> > +			&& n->n_namesz > data->d_size
> > +			&& n->n_descsz > data->d_size))
> > +	    {
> > +	      /* n might not be writable, use result and redirect n.  */
> > +	      *result = *n;
> > +	      result->n_type = bswap_32 (n->n_type);
> > +	      result->n_namesz = bswap_32 (n->n_namesz);
> > +	      result->n_descsz = bswap_32 (n->n_descsz);
> > +	      n = result;
> > +	    }
> > +
>
> Thanks, but I don't think it's necessary to apply that just now - this
> is a bug and I'll get a fix in the script first in the next couple
> days, and then I'll chat with the Fedora dev working on this about what
> to do regarding existing binaries.

I did apply it to the Fedora package already:
https://src.fedoraproject.org/rpms/elfutils/blob/rawhide/f/elfutils-0.186-fdo-swap.patch
Without it almost all of the selftests fail. And it seems a lot of
binaries on Fedora have already been compiled with this bogus ELF
notes in it. The trouble with that is that the notes themselves are
bad (the sizes are garbage, so anything trying to parse them will fail
and will be unable to parse any notes in the same segement.

> > Note that Tom (on CC) submitted an IMHO much saner way to generate the
> > package notes using simple assembly which would have gotten all this
> > correct automagically.
> > https://src.fedoraproject.org/fork/tstellar/rpms/package-notes/c/25687ec2d8a4262d5ba5c55d35d68a994b892910
> > 
> > I see you rejected that, but please reconsider. Just hardcoding some
> > byte values really is broken.
> 
> The reality of having to deal with thirty thousand different build
> system, integrated with different tools, and different packaging
> systems, with different build scripts, on different distros, means that
> ease of integration trumps over everything else. There are packages out
> there using build systems that you couldn't even imagine in your worst
> nightmares :-)

I can imagine that, but to be honest I think that is precisely because
you are using a linker script. Best would be to make sure there is
native support in the linker for this, just like linkers have native
support for build-ids. Otherwise linking in a simple assembly
generated note seems a good idea. Linker scripts seem the most
fragile.

But if you insist using linker script please use the proper BYTE,
SHORT, LONG directives to store the ELF note structure values, instead
of a stream of BYTEs, so the linker can take care of the correct value
(endianness) representation for the target arch.

Cheers,

Mark
  

On Sat, 2022-03-26 at 17:33 +0100, Mark Wielaard wrote:
> Hi Luca,
> 
> On Fri, Mar 25, 2022 at 02:55:14PM +0000, Luca Boccassi wrote:
> > On Fri, 2022-03-25 at 15:47 +0100, Mark Wielaard wrote:
> > > > We have completely overlooked that, the note is created by a
> > > > linker
> > > > script, which is generated at build time by this:
> > > > 
> > > > https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254
> > > > 
> > > > (well an older version in Fedora, but similar enough)
> > > 
> > > Yeah, that is definitely wrong. ELF is very careful about
> > > endianess. I
> > > have a patch that detects it and works around it. But it is
> > > somewhat
> > > ugly and has to work very low-level. So I am not sure I really
> > > want it.
> > > Maybe I just apply it as a temporary workaround just for Fedora.
> > > But if
> > > other distros have used such a script to generate package notes
> > > they
> > > are also broken.
> > > 
> > > diff --git a/libelf/gelf_getnote.c b/libelf/gelf_getnote.c
> > > [...]
> > > @@ -73,6 +74,22 @@ gelf_getnote (Elf_Data *data, size_t offset,
> > > GElf_Nhdr *result,
> > >         offset = 0;
> > >        else
> > >         {
> > > +         /* Workaround FDO package notes on big-endian systems,
> > > +            getting namesz and descsz wrong. Detect it by
> > > getting
> > > +            a bad namesz, descsz and byte swapped n_type for
> > > +            NT_FDO_PACKAGING_METADATA.  */
> > > +         if (unlikely (n->n_type == bswap_32
> > > (NT_FDO_PACKAGING_METADATA)
> > > +                       && n->n_namesz > data->d_size
> > > +                       && n->n_descsz > data->d_size))
> > > +           {
> > > +             /* n might not be writable, use result and redirect
> > > n.  */
> > > +             *result = *n;
> > > +             result->n_type = bswap_32 (n->n_type);
> > > +             result->n_namesz = bswap_32 (n->n_namesz);
> > > +             result->n_descsz = bswap_32 (n->n_descsz);
> > > +             n = result;
> > > +           }
> > > +
> > 
> > Thanks, but I don't think it's necessary to apply that just now -
> > this
> > is a bug and I'll get a fix in the script first in the next couple
> > days, and then I'll chat with the Fedora dev working on this about
> > what
> > to do regarding existing binaries.
> 
> I did apply it to the Fedora package already:
> https://src.fedoraproject.org/rpms/elfutils/blob/rawhide/f/elfutils-0.186-fdo-swap.patch
> Without it almost all of the selftests fail. And it seems a lot of
> binaries on Fedora have already been compiled with this bogus ELF
> notes in it. The trouble with that is that the notes themselves are
> bad (the sizes are garbage, so anything trying to parse them will
> fail
> and will be unable to parse any notes in the same segement.

Thank you - if we end up rebuilding s390x I'll let you know. The
current segment is clearly bogus, so I'll suggest we do that once the
script is fixed (working on that).

> > > Note that Tom (on CC) submitted an IMHO much saner way to
> > > generate the
> > > package notes using simple assembly which would have gotten all
> > > this
> > > correct automagically.
> > > https://src.fedoraproject.org/fork/tstellar/rpms/package-notes/c/25687ec2d8a4262d5ba5c55d35d68a994b892910
> > > 
> > > I see you rejected that, but please reconsider. Just hardcoding
> > > some
> > > byte values really is broken.
> > 
> > The reality of having to deal with thirty thousand different build
> > system, integrated with different tools, and different packaging
> > systems, with different build scripts, on different distros, means
> > that
> > ease of integration trumps over everything else. There are packages
> > out
> > there using build systems that you couldn't even imagine in your
> > worst
> > nightmares :-)
> 
> I can imagine that, but to be honest I think that is precisely
> because
> you are using a linker script. Best would be to make sure there is
> native support in the linker for this, just like linkers have native
> support for build-ids. Otherwise linking in a simple assembly
> generated note seems a good idea. Linker scripts seem the most
> fragile.
> 
> But if you insist using linker script please use the proper BYTE,
> SHORT, LONG directives to store the ELF note structure values,
> instead
> of a stream of BYTEs, so the linker can take care of the correct
> value
> (endianness) representation for the target arch.

Generating a binary is actually harder, we tried that first, there is
just too much variation and completely wonky build systems out there.
Already working on the updated script, the native type is exactly the
approach I was taking, this works fine on a Debian machine on s390x
(and also on x86_64), eg:

-        BYTE(0x04) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of Owner including NUL */
-        BYTE(0x47) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of Value including NUL */
-        BYTE(0x7e) BYTE(0x1a) BYTE(0xfe) BYTE(0xca) /* Note ID */
+        LONG(0x04)                                  /* Length of Owner including NUL */
+        LONG(0x0047)                                /* Length of Value including NUL */
+        LONG(0xcafe1a7e)                            /* Note ID */

The rest of the fields are C strings so no change needed, I believe.

Does this look right to you as well?
  

On Sat, 2022-03-26 at 16:57 +0000, Luca Boccassi wrote:
> On Sat, 2022-03-26 at 17:33 +0100, Mark Wielaard wrote:
> > Hi Luca,
> > 
> > On Fri, Mar 25, 2022 at 02:55:14PM +0000, Luca Boccassi wrote:
> > > On Fri, 2022-03-25 at 15:47 +0100, Mark Wielaard wrote:
> > > > > We have completely overlooked that, the note is created by a
> > > > > linker
> > > > > script, which is generated at build time by this:
> > > > > 
> > > > > https://github.com/systemd/package-notes/blob/main/generate-package-notes.sh#L254
> > > > > 
> > > > > (well an older version in Fedora, but similar enough)
> > > > 
> > > > Yeah, that is definitely wrong. ELF is very careful about
> > > > endianess. I
> > > > have a patch that detects it and works around it. But it is
> > > > somewhat
> > > > ugly and has to work very low-level. So I am not sure I really
> > > > want it.
> > > > Maybe I just apply it as a temporary workaround just for
> > > > Fedora.
> > > > But if
> > > > other distros have used such a script to generate package notes
> > > > they
> > > > are also broken.
> > > > 
> > > > diff --git a/libelf/gelf_getnote.c b/libelf/gelf_getnote.c
> > > > [...]
> > > > @@ -73,6 +74,22 @@ gelf_getnote (Elf_Data *data, size_t offset,
> > > > GElf_Nhdr *result,
> > > >         offset = 0;
> > > >        else
> > > >         {
> > > > +         /* Workaround FDO package notes on big-endian
> > > > systems,
> > > > +            getting namesz and descsz wrong. Detect it by
> > > > getting
> > > > +            a bad namesz, descsz and byte swapped n_type for
> > > > +            NT_FDO_PACKAGING_METADATA.  */
> > > > +         if (unlikely (n->n_type == bswap_32
> > > > (NT_FDO_PACKAGING_METADATA)
> > > > +                       && n->n_namesz > data->d_size
> > > > +                       && n->n_descsz > data->d_size))
> > > > +           {
> > > > +             /* n might not be writable, use result and
> > > > redirect
> > > > n.  */
> > > > +             *result = *n;
> > > > +             result->n_type = bswap_32 (n->n_type);
> > > > +             result->n_namesz = bswap_32 (n->n_namesz);
> > > > +             result->n_descsz = bswap_32 (n->n_descsz);
> > > > +             n = result;
> > > > +           }
> > > > +
> > > 
> > > Thanks, but I don't think it's necessary to apply that just now -
> > > this
> > > is a bug and I'll get a fix in the script first in the next
> > > couple
> > > days, and then I'll chat with the Fedora dev working on this
> > > about
> > > what
> > > to do regarding existing binaries.
> > 
> > I did apply it to the Fedora package already:
> > https://src.fedoraproject.org/rpms/elfutils/blob/rawhide/f/elfutils-0.186-fdo-swap.patch
> > Without it almost all of the selftests fail. And it seems a lot of
> > binaries on Fedora have already been compiled with this bogus ELF
> > notes in it. The trouble with that is that the notes themselves are
> > bad (the sizes are garbage, so anything trying to parse them will
> > fail
> > and will be unable to parse any notes in the same segement.
> 
> Thank you - if we end up rebuilding s390x I'll let you know. The
> current segment is clearly bogus, so I'll suggest we do that once the
> script is fixed (working on that).
> 
> > > > Note that Tom (on CC) submitted an IMHO much saner way to
> > > > generate the
> > > > package notes using simple assembly which would have gotten all
> > > > this
> > > > correct automagically.
> > > > https://src.fedoraproject.org/fork/tstellar/rpms/package-notes/c/25687ec2d8a4262d5ba5c55d35d68a994b892910
> > > > 
> > > > I see you rejected that, but please reconsider. Just hardcoding
> > > > some
> > > > byte values really is broken.
> > > 
> > > The reality of having to deal with thirty thousand different
> > > build
> > > system, integrated with different tools, and different packaging
> > > systems, with different build scripts, on different distros,
> > > means
> > > that
> > > ease of integration trumps over everything else. There are
> > > packages
> > > out
> > > there using build systems that you couldn't even imagine in your
> > > worst
> > > nightmares :-)
> > 
> > I can imagine that, but to be honest I think that is precisely
> > because
> > you are using a linker script. Best would be to make sure there is
> > native support in the linker for this, just like linkers have
> > native
> > support for build-ids. Otherwise linking in a simple assembly
> > generated note seems a good idea. Linker scripts seem the most
> > fragile.
> > 
> > But if you insist using linker script please use the proper BYTE,
> > SHORT, LONG directives to store the ELF note structure values,
> > instead
> > of a stream of BYTEs, so the linker can take care of the correct
> > value
> > (endianness) representation for the target arch.
> 
> Generating a binary is actually harder, we tried that first, there is
> just too much variation and completely wonky build systems out there.
> Already working on the updated script, the native type is exactly the
> approach I was taking, this works fine on a Debian machine on s390x
> (and also on x86_64), eg:
> 
> -        BYTE(0x04) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> Owner including NUL */
> -        BYTE(0x47) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> Value including NUL */
> -        BYTE(0x7e) BYTE(0x1a) BYTE(0xfe) BYTE(0xca) /* Note ID */
> +        LONG(0x04)                                  /* Length of
> Owner including NUL */
> +        LONG(0x0047)                                /* Length of
> Value including NUL */
> +        LONG(0xcafe1a7e)                            /* Note ID */
> 
> The rest of the fields are C strings so no change needed, I believe.
> 
> Does this look right to you as well?

Here's the fix:

https://src.fedoraproject.org/rpms/package-notes/pull-request/3#

Now it's up to the Fedora folks what to do with it. I tested the
updated script on Debian x86_64 and s390x, and it all works as
intended. Sorry again for the breakage!
  

Hi Luca,

On Sat, 2022-03-26 at 18:19 +0000, Luca Boccassi wrote:
> > Already working on the updated script, the native type is exactly
> > the
> > approach I was taking, this works fine on a Debian machine on s390x
> > (and also on x86_64), eg:
> > 
> > -        BYTE(0x04) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> > Owner including NUL */
> > -        BYTE(0x47) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> > Value including NUL */
> > -        BYTE(0x7e) BYTE(0x1a) BYTE(0xfe) BYTE(0xca) /* Note ID */
> > +        LONG(0x04)                                  /* Length of
> > Owner including NUL */
> > +        LONG(0x0047)                                /* Length of
> > Value including NUL */
> > +        LONG(0xcafe1a7e)                            /* Note ID */
> > 
> > The rest of the fields are C strings so no change needed, I
> > believe.
> > 
> > Does this look right to you as well?
> 
> Here's the fix:
> 
> https://src.fedoraproject.org/rpms/package-notes/pull-request/3#
> 
> Now it's up to the Fedora folks what to do with it. I tested the
> updated script on Debian x86_64 and s390x, and it all works as
> intended. Sorry again for the breakage!

Yes, that looks correct. Note that the example on 
https://systemd.io/COREDUMP_PACKAGE_METADATA/ also uses BYTEs for
everything, instead of LONGs for the namesz, descsz and type words.

This also seems to make sure everything is aligned (at 4 bytes). An ELF
note is defined as an array of (4 byte) words. Where the first 3
(n_namesz, n_descsz, n_type) have a special interpretation. Your name
string is also exactly 4 bytes "FDO\0", so you don't need any extra
padding to make the start of the descriptor be aligned. And since you
don't add any other notes to the section you don't need to explicitly
pad the description. The linker should take care of that in case it
merges note sections/segments.

Still I would really recommend trying to add native support to linkers
for package notes, just like they support build-ids by default. That
also makes it easier for the linker to simply merge the notes. Trying
to do this with inserting a linker script really feels very fragile.

Cheers,

Mark
  

On Mon, 2022-03-28 at 11:57 +0200, Mark Wielaard wrote:
> Hi Luca,
> 
> On Sat, 2022-03-26 at 18:19 +0000, Luca Boccassi wrote:
> > > Already working on the updated script, the native type is exactly
> > > the
> > > approach I was taking, this works fine on a Debian machine on s390x
> > > (and also on x86_64), eg:
> > > 
> > > -        BYTE(0x04) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> > > Owner including NUL */
> > > -        BYTE(0x47) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of
> > > Value including NUL */
> > > -        BYTE(0x7e) BYTE(0x1a) BYTE(0xfe) BYTE(0xca) /* Note ID */
> > > +        LONG(0x04)                                  /* Length of
> > > Owner including NUL */
> > > +        LONG(0x0047)                                /* Length of
> > > Value including NUL */
> > > +        LONG(0xcafe1a7e)                            /* Note ID */
> > > 
> > > The rest of the fields are C strings so no change needed, I
> > > believe.
> > > 
> > > Does this look right to you as well?
> > 
> > Here's the fix:
> > 
> > https://src.fedoraproject.org/rpms/package-notes/pull-request/3#
> > 
> > Now it's up to the Fedora folks what to do with it. I tested the
> > updated script on Debian x86_64 and s390x, and it all works as
> > intended. Sorry again for the breakage!
> 
> Yes, that looks correct. Note that the example on 
> https://systemd.io/COREDUMP_PACKAGE_METADATA/ also uses BYTEs for
> everything, instead of LONGs for the namesz, descsz and type words.
> 
> This also seems to make sure everything is aligned (at 4 bytes). An ELF
> note is defined as an array of (4 byte) words. Where the first 3
> (n_namesz, n_descsz, n_type) have a special interpretation. Your name
> string is also exactly 4 bytes "FDO\0", so you don't need any extra
> padding to make the start of the descriptor be aligned. And since you
> don't add any other notes to the section you don't need to explicitly
> pad the description. The linker should take care of that in case it
> merges note sections/segments.

Fix for the doc is queued too:
https://github.com/systemd/systemd/pull/22879

> Still I would really recommend trying to add native support to linkers
> for package notes, just like they support build-ids by default. That
> also makes it easier for the linker to simply merge the notes. Trying
> to do this with inserting a linker script really feels very fragile.

Yes, that would definitely be ideal. But a bit of a chicken-and-egg: we
had to get the spec established before there's any realistic change of
getting a full new feature merged in BFD, but we can't get it
established if we can't use it anywhere. This way we can 'bootstrap'
ourselves, and once the idea and usage is more widely accepted, we have
a good case to do just that.