[PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[Andrei Homescu]

The mkl_memory_patched.o object inside the libmkl_core.a library from
the Intel Math Kernel Library version 2018.2.199 has this section
with an alignment of 4096 and offset of 0xb68:
 [ 2] .data PROGBITS 0000000000000000 000b68 011000 00 WA 0 0 4096

Reading this file with libelf and trying to write it back to disk triggers
the following sequence of events:
1) code in elf_getdata.c clamps d_align for this section's data buffer
   to the section's offset
2) code in elf32_updatenull.c checks if the alignment is a power of two
   and incorrectly returns an error

This commit fixes this corner case by increasing the alignment to the
next power of two after the clamping, so the check passes.

A test that reproduces this bug using strip is also included.

Signed-off-by: Andrei Homescu <ah@immunant.com>
---
 libelf/ChangeLog                |   5 +++++
 libelf/elf_getdata.c            |  13 +++++++++++-
 tests/ChangeLog                 |   7 +++++++
 tests/Makefile.am               |   5 +++--
 tests/run-strip-largealign.sh   |  34 ++++++++++++++++++++++++++++++++
 tests/testfile-largealign.o.bz2 | Bin 0 -> 183 bytes
 6 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100755 tests/run-strip-largealign.sh
 create mode 100644 tests/testfile-largealign.o.bz2

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index a1fd414c..6144ee78 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-27  Andrei Homescu  <ah@immunant.com>
+
+	* elf_getdata.c: Fix d_align for sections where alignment is larger
+	than offset.
+
 2020-12-12  Mark Wielaard  <mark@klomp.org>
 
 	* elf.h: Update from glibc.
diff --git a/libelf/elf_getdata.c b/libelf/elf_getdata.c
index 6ed44504..3cad29de 100644
--- a/libelf/elf_getdata.c
+++ b/libelf/elf_getdata.c
@@ -384,7 +384,18 @@ __libelf_set_rawdata_wrlock (Elf_Scn *scn)
      which should be uncommon.  */
   align = align ?: 1;
   if (type != SHT_NOBITS && align > offset)
-    align = offset;
+    {
+      /* Align the offset to the next power of two. Uses algorithm from
+         https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2 */
+      align = offset - 1;
+      align |= align >> 1;
+      align |= align >> 2;
+      align |= align >> 4;
+      align |= align >> 8;
+      align |= align >> 16;
+      align |= align >> 32;
+      align++;
+    }
   scn->rawdata.d.d_align = align;
   if (elf->class == ELFCLASS32
       || (offsetof (struct Elf, state.elf32.ehdr)
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 38e92659..12a6dc7f 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,10 @@
+2021-06-09  Andrei Homescu  <ah@immunant.com>
+
+	* testfile-largealign.o.bz2: New test file.
+	* run-strip-largealign.sh: New test.
+	* Makefile.am (TESTS): Add run-strip-largealign.sh.
+	(EXTRA_DIST): Add run-strip-largealign.sh and testfile-largealign.o.bz2
+
 2021-05-14  Frank Ch. Eigler <fche@redhat.com>
 
 	PR27859
diff --git a/tests/Makefile.am b/tests/Makefile.am
index dc7517d9..e93f0c2c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -188,7 +188,7 @@ TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \
 	leb128 read_unaligned \
 	msg_tst system-elf-libelf-test \
 	$(asm_TESTS) run-disasm-bpf.sh run-low_high_pc-dw-form-indirect.sh \
-	run-readelf-dw-form-indirect.sh
+	run-readelf-dw-form-indirect.sh run-strip-largealign.sh
 
 if !BIARCH
 export ELFUTILS_DISABLE_BIARCH = 1
@@ -509,7 +509,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     testfile_pt_gnu_prop.bz2 testfile_pt_gnu_prop32.bz2 \
 	     run-getphdrnum.sh testfile-phdrs.elf.bz2 \
 	     run-test-includes.sh run-low_high_pc-dw-form-indirect.sh \
-	     run-readelf-dw-form-indirect.sh testfile-dw-form-indirect.bz2
+	     run-readelf-dw-form-indirect.sh testfile-dw-form-indirect.bz2 \
+	     testfile-largealign.bz2 run-strip-largealign.sh
 
 
 if USE_VALGRIND
diff --git a/tests/run-strip-largealign.sh b/tests/run-strip-largealign.sh
new file mode 100755
index 00000000..897ca5ba
--- /dev/null
+++ b/tests/run-strip-largealign.sh
@@ -0,0 +1,34 @@
+#! /bin/sh
+# Copyright (C) 2011 TODO.
+# This file is part of elfutils.
+#
+# This file is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# elfutils is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+. $srcdir/test-subr.sh
+
+# = testfile-largealign.S =
+# section .data
+# align 4096
+# dd 0x12345678
+#
+# nasm -f elf64 -o testfile-largealign.o testfile-largealign.S
+
+infile=testfile-largealign.o
+outfile=$infile.stripped
+
+testfiles $infile
+tempfiles $outfile
+
+testrun ${abs_top_builddir}/src/strip -o $outfile $infile
diff --git a/tests/testfile-largealign.o.bz2 b/tests/testfile-largealign.o.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..324c1eae376dab373d4860188d1c29d577b64abc
GIT binary patch
literal 183
zcmV;o07(BrT4*^jL0KkKS$lTN?*IUO|K$Gs@n9qY1_MJx2>>=i9ALlzKmb4hAON5M
zfHcu5lL%>%qalz6gBn8_42FY2qd*z}8W~!#9HOGvK}7V;m`k9kmnQ7Xs*(clq9*|2
zA}D|*im9-mNr*r@N){aWV)s@c9Zj;%)REkP6oAtzQc;Q^velES!XQ@Wspq}yD~@;F
lCD&sZA1p^UFHqN<4rm_o_X>?mK--t`cO+AV2?w`qzVN>OO*jAm

literal 0
HcmV?d00001

-- 
2.31.1

Re: [PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[Mark Wielaard]

Hi Andrei,

On Mon, 2021-06-28 at 18:26 -0700, Andrei Homescu wrote:
> The mkl_memory_patched.o object inside the libmkl_core.a library from
> the Intel Math Kernel Library version 2018.2.199 has this section
> with an alignment of 4096 and offset of 0xb68:
>  [ 2] .data PROGBITS 0000000000000000 000b68 011000 00 WA 0 0 4096
> 
> Reading this file with libelf and trying to write it back to disk triggers
> the following sequence of events:
> 1) code in elf_getdata.c clamps d_align for this section's data buffer
>    to the section's offset
> 2) code in elf32_updatenull.c checks if the alignment is a power of two
>    and incorrectly returns an error
> 
> This commit fixes this corner case by increasing the alignment to the
> next power of two after the clamping, so the check passes.
> 
> A test that reproduces this bug using strip is also included.

Thanks for this perfect patch, including and excellent explanation of
the issue and a testcase. I have only a minor suggestion:

> --- /dev/null
> +++ b/tests/run-strip-largealign.sh
> @@ -0,0 +1,34 @@
> +#! /bin/sh
> +# Copyright (C) 2011 TODO.

Should we make this Copyright (C) 2021 Andrei Homescu <ah@immunant.com>
or Copyright (C) 2021 Immunant, Inc.

> +# This file is part of elfutils.
> +#
> +# This file is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published
> by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# elfutils is distributed in the hope that it will be useful, but
> +# WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <
> http://www.gnu.org/licenses/>.
> +#
> +
> +. $srcdir/test-subr.sh
> +
> +# = testfile-largealign.S =
> +# section .data
> +# align 4096
> +# dd 0x12345678
> +#
> +# nasm -f elf64 -o testfile-largealign.o testfile-largealign.S
> +
> +infile=testfile-largealign.o
> +outfile=$infile.stripped
> +
> +testfiles $infile
> +tempfiles $outfile
> +
> +testrun ${abs_top_builddir}/src/strip -o $outfile $infile

The testcase already fails before the patch and succeeds after, but it
would be nice to double check the output with elflint just in case.
Shall we add:

testrun ${abs_top_builddir}/src/elflint --gnu $outfile

Thanks,

Mark

Re: [PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[Andrei Homescu]

Hi,

On Wed, Jul 7, 2021 at 5:02 AM Mark Wielaard <mark@klomp.org> wrote:

> Hi Andrei,
>
> On Mon, 2021-06-28 at 18:26 -0700, Andrei Homescu wrote:
> > The mkl_memory_patched.o object inside the libmkl_core.a library from
> > the Intel Math Kernel Library version 2018.2.199 has this section
> > with an alignment of 4096 and offset of 0xb68:
> >  [ 2] .data PROGBITS 0000000000000000 000b68 011000 00 WA 0 0 4096
> >
> > Reading this file with libelf and trying to write it back to disk
> triggers
> > the following sequence of events:
> > 1) code in elf_getdata.c clamps d_align for this section's data buffer
> >    to the section's offset
> > 2) code in elf32_updatenull.c checks if the alignment is a power of two
> >    and incorrectly returns an error
> >
> > This commit fixes this corner case by increasing the alignment to the
> > next power of two after the clamping, so the check passes.
> >
> > A test that reproduces this bug using strip is also included.
>
> Thanks for this perfect patch, including and excellent explanation of
> the issue and a testcase. I have only a minor suggestion:
>
> > --- /dev/null
> > +++ b/tests/run-strip-largealign.sh
> > @@ -0,0 +1,34 @@
> > +#! /bin/sh
> > +# Copyright (C) 2011 TODO.
>
> Should we make this Copyright (C) 2021 Andrei Homescu <ah@immunant.com>
> or Copyright (C) 2021 Immunant, Inc.
>
Sorry about that, I intended to resolve that TODO myself and then missed
it.
I wrote this patch on behalf of someone else, so the copyright line should
be:
Copyright (C) 2021 Runsafe Security, Inc.


> > +# This file is part of elfutils.
> > +#
> > +# This file is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published
> > by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# elfutils is distributed in the hope that it will be useful, but
> > +# WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <
> > http://www.gnu.org/licenses/>.
> > +#
> > +
> > +. $srcdir/test-subr.sh
> > +
> > +# = testfile-largealign.S =
> > +# section .data
> > +# align 4096
> > +# dd 0x12345678
> > +#
> > +# nasm -f elf64 -o testfile-largealign.o testfile-largealign.S
> > +
> > +infile=testfile-largealign.o
> > +outfile=$infile.stripped
> > +
> > +testfiles $infile
> > +tempfiles $outfile
> > +
> > +testrun ${abs_top_builddir}/src/strip -o $outfile $infile
>
> The testcase already fails before the patch and succeeds after, but it
> would be nice to double check the output with elflint just in case.
> Shall we add:
>
> testrun ${abs_top_builddir}/src/elflint --gnu $outfile
>
Sounds good, no objection from me.
Should I submit an updated version of the patch, or can you add this on
your end?

Andrei

Re: [PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[Mark Wielaard]

Hi Andrei,

On Wed, 2021-07-07 at 13:24 -0700, Andrei Homescu wrote:
> I wrote this patch on behalf of someone else, so the copyright line
> should be:
> Copyright (C) 2021 Runsafe Security, Inc.

OK, changed.

> > The testcase already fails before the patch and succeeds after, but
> > it would be nice to double check the output with elflint just in
> > case. Shall we add:
> > 
> > testrun ${abs_top_builddir}/src/elflint --gnu $outfile
> 
> Sounds good, no objection from me.
> Should I submit an updated version of the patch, or can you add this
> on your end?

I made those two tweaks and pushed as:

https://sourceware.org/git/?p=elfutils.git;a=commit;h=b3601167d7a4c9f34eb65c3892c9ef25e3c1c66f

Thanks,

Mark

Re: [PATCH] libelf: Fix unaligned d_off offsets for input sections with large alignments

[Andrei Homescu]

On Thu, Jul 8, 2021 at 2:37 AM Mark Wielaard <mark@klomp.org> wrote:

> Hi Andrei,
>
> On Wed, 2021-07-07 at 13:24 -0700, Andrei Homescu wrote:
> > I wrote this patch on behalf of someone else, so the copyright line
> > should be:
> > Copyright (C) 2021 Runsafe Security, Inc.
>
> OK, changed.
>
> > > The testcase already fails before the patch and succeeds after, but
> > > it would be nice to double check the output with elflint just in
> > > case. Shall we add:
> > >
> > > testrun ${abs_top_builddir}/src/elflint --gnu $outfile
> >
> > Sounds good, no objection from me.
> > Should I submit an updated version of the patch, or can you add this
> > on your end?
>
> I made those two tweaks and pushed as:
>
>
> https://sourceware.org/git/?p=elfutils.git;a=commit;h=b3601167d7a4c9f34eb65c3892c9ef25e3c1c66f
>
> Thanks,
>
> Mark
>

That's great, thank you!
Andrei