[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