Re: [PATCH v2] libebl: recognize FDO Packaging Metadata ELF note

[Luca Boccassi <bluca@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 5720 bytes --]

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?

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]