Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 579388 - sys-devel/prelink-20151030: fix crash (segmentation fault) etc
Summary: sys-devel/prelink-20151030: fix crash (segmentation fault) etc
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal with 1 vote (vote)
Assignee: Gentoo Toolchain Maintainers
URL:
Whiteboard:
Keywords: PMASKED
Depends on:
Blocks:
 
Reported: 2016-04-08 22:52 UTC by Alexander Miller
Modified: 2022-02-23 09:40 UTC (History)
2 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
prelink-20151030-gather_c-maybe_pie-oob-access.patch (prelink-20151030-gather_c-maybe_pie-oob-access.patch,6.05 KB, patch)
2016-04-08 22:52 UTC, Alexander Miller
Details | Diff
prelink-20151030-libiberty-sha.patch (prelink-20151030-libiberty-sha.patch,1.23 KB, patch)
2016-04-08 23:02 UTC, Alexander Miller
Details | Diff
prelink-20151030-dynamic_linker_alt.patch (prelink-20151030-dynamic_linker_alt.patch,2.00 KB, patch)
2016-04-08 23:17 UTC, Alexander Miller
Details | Diff
prelink-20151030-gather_c-maybe_pie-oob-access.patch V2 (prelink-20151030-gather_c-maybe_pie-oob-access.patch,5.48 KB, patch)
2016-05-06 13:24 UTC, Alexander Miller
Details | Diff
prelink-20151030-libiberty-sha.patch (libiberty-sha.patch,1.23 KB, patch)
2017-08-26 22:14 UTC, Alexander Miller
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Miller 2016-04-08 22:52:49 UTC
Created attachment 429954 [details, diff]
prelink-20151030-gather_c-maybe_pie-oob-access.patch

On my amd64 box prelink would always segfault while scanning files.
In my case /usr/lib64/jvm/icedtea-bin-7/jre/lib/amd64/xawt/libmawt.so
(which is really /opt/icedtea-bin-7.2.6.5/jre/lib/amd64/xawt/libmawt.so)
would cause the crash.

I located the bug and sent a patch upstream (see
<https://lists.yoctoproject.org/pipermail/yocto/2016-April/029385.html>).
I got no answer yet, but this should probably fixed asap for other gentoo
users. So I'm attaching my patch here, too.
Comment 1 Alexander Miller 2016-04-08 23:02:36 UTC
Created attachment 429956 [details, diff]
prelink-20151030-libiberty-sha.patch

Although not directly related, I'm posting my other patches for this
package, too. When the ebuild needs updating anyway, you can add them
all in one bump.

This patch fixes a QA warning for sha.c and is an adaption of the existing
<https://gitweb.gentoo.org/repo/gentoo.git/tree/sys-devel/prelink/files/prelink-20130503-libiberty-md5.patch>
which fixes a similar issue for md5.c
Comment 2 Alexander Miller 2016-04-08 23:17:38 UTC
Created attachment 429958 [details, diff]
prelink-20151030-dynamic_linker_alt.patch

Finally, this patch avoids errors like:
  /usr/sbin/prelink: /usr/lib32/misc/glibc/getconf/XBS5_ILP32_OFF32: Using /lib32/ld-linux.so.2, not /lib/ld-linux.so.2 as dynamic linker
by adding alt. dynamic linker paths. The two dynamic linkers are in fact
the same file, one a symlink to the other. In gentoo, there are many possible
libdir layouts, and this way we can cover them all. (I've added them for
amd64, x86, ppc, pp64 since those are the keywords listed in the ebuild).
Comment 3 Alexander Miller 2016-04-08 23:23:48 UTC
You should also call epatch_user (or eapply_user) in the ebuild.
Then we could test patches without resorting to ugly hacks.
Comment 4 SpanKY gentoo-dev 2016-05-05 05:55:34 UTC
Comment on attachment 429958 [details, diff]
prelink-20151030-dynamic_linker_alt.patch

i think the only hunk we want is the i386 one, and only when SYMLINK_LIB=yes.  we're trying to phase that out, so for non-lib32 x86 setups, we shouldn't need any changes.
Comment 5 SpanKY gentoo-dev 2016-05-05 06:10:15 UTC
Comment on attachment 429954 [details, diff]
prelink-20151030-gather_c-maybe_pie-oob-access.patch

i don't think this code really fixes the problem.  there is no requirement that the program headers immediately follow the Elf header which is what the code previously assumed, and in this version still assumes.

basically this part:
+      unsigned char *phoff = e_ident + offsetof (Elf64_Ehdr, e_phoff);
...
+        phdr_offset = buf_read_ube64 (phoff);
you've already lost :).

instead of trying to stuff the program header into e_ident at all, the code should be entirely based on pread and offsets.  if performance is a concern, the code should mmap the part of the file that contains the program headers and then walk it in memory -- you'll know for sure the start via ehdr->e_phoff and the end by using e_phoff + e_phnum * e_phentsize.  the latter two are 16bit values, so you don't have to worry about overflow: just verify that (stat.st_size - e_phnum * e_phentsize <= e_phoff).
Comment 6 Alexander Miller 2016-05-05 19:27:59 UTC
No, the code doesn't assume that the program headers immediately follow the Elf header, that's the whole point of the patch. I think you misread that part of the code. The snippet you cited simply reads the program header's offset from the Elf header (ehdr->e_phoff). I can't see what's wrong with it.

Reading the first program headers from e_ident *if* they immediately follow the Elf header comes for free, so I don't see a problem with that optimization. Removing the feature wouldn't simplify the code at all.

We could use mmap instead of pread, but unless there are real performance concerns I'd go with the existing implementation.

On my comment about overflow: of course I didn't worry about the 16bit values, but the addition can overflow; the stat.st_size check you suggest is basically what I meant (except you got the comparison wrong). In fact I was thinking about a purely numeric check, but that's complicated because there is no OFF_MAX - I did't think about stat, and the overhead of calling it seems acceptable. So we might replace the TODO comment with:
  else
    {
      struct stat st;
      if (fstat(fd, &st) || (st.st_size - num_phdrs * phentsize < phdr_offset))
        return 0;
    }
(Note: we may also check if (st.st_size < num_phdrs * phentsize) immediately or simply wait until pread fails.)

Maybe I should mention that the code doesn't handle more than 64k program headers. In that case we simply give up after the first 64k.

Do you still have objections? Should I create an updated patch with the overflow check added or can you do that yourself?
Comment 7 Alexander Miller 2016-05-05 19:48:38 UTC
Oh, I hit "Save Changes" too early. Just ignore this:
> (Note: we may also check if (st.st_size < num_phdrs * phentsize) immediately or simply wait until pread fails.)
I forgot to delete it after fixing the comparison in the code snippet.
Comment 8 Alexander Miller 2016-05-06 13:24:09 UTC
Created attachment 433408 [details, diff]
prelink-20151030-gather_c-maybe_pie-oob-access.patch V2

This is a new version of my patch with the check added (of course we still need a cast to avoid overflow, you totally confused me with your comment). I've also changed phdr_offset into an explicit 64bit type.

I still think we shouldn't care about the e_phnum == PN_XNUM case.
Comment 9 Alexander Miller 2017-08-26 22:14:11 UTC
Created attachment 490764 [details, diff]
prelink-20151030-libiberty-sha.patch

Replace broken patch.

I haven't noticed it was broken all the time since I was using
the correct patch locally. Don't know how I managed to upload
a broken file in the first place. Sorry for the inconvenience.
Comment 10 Sergei Trofimovich (RETIRED) gentoo-dev 2019-11-24 21:55:10 UTC
Are these patches still relevant? I suggest working directly with upstream to get these patches incorporated:
    https://git.yoctoproject.org/cgit/cgit.cgi/prelink-cross/

We can cut an upstream snapshot to use it in gentoo.
Comment 11 Alexander Miller 2019-11-25 22:07:19 UTC
I haven't been following prelink development for quite some time, so I don't know whether the patches are still relevant. I had a quick look at the commit log and haven't seen anything relatated, though.
The alternative dynamic linker locations may be too Gentoo-specific for upstream. The libiberty hash algorithms don't seem to have been updated, so I guess the patches for md5 (in gentoo) and sha1 (here) are still relevant; prelink should refresh these files from libiberty.
Concerning the patch fixing the oob access, the last thing I heard from upstream was that Mark Hatle wanted to go through the pending patches (including mine) and get them reviewed and merged. That was in June 2016. No updates or comments ever since. I guess it got lost in a pile of other stuff and I didn't revisit the issue either.
Comment 12 Andreas K. Hüttel archtester gentoo-dev 2022-01-22 01:01:58 UTC
Prelink support is being dropped upstream in glibc-2.35; sys-devel/prelink has been masked for removal.
Comment 13 Larry the Git Cow gentoo-dev 2022-02-23 09:40:36 UTC
The bug has been closed via the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=9e73effa51918eb614a66f4fa7670fded98a0d66

commit 9e73effa51918eb614a66f4fa7670fded98a0d66
Author:     Jakov Smolić <jsmolic@gentoo.org>
AuthorDate: 2022-02-23 09:20:03 +0000
Commit:     Jakov Smolić <jsmolic@gentoo.org>
CommitDate: 2022-02-23 09:40:00 +0000

    sys-devel/prelink: treeclean
    
    Closes: https://bugs.gentoo.org/579388
    Closes: https://bugs.gentoo.org/726062
    Signed-off-by: Jakov Smolić <jsmolic@gentoo.org>

 profiles/package.mask                              |  1 -
 sys-devel/prelink/Manifest                         |  2 -
 .../files/prelink-20130503-libiberty-md5.patch     | 61 ------------------
 .../files/prelink-20130503-prelink-conf.patch      | 39 ------------
 sys-devel/prelink/files/prelink.confd              | 43 -------------
 sys-devel/prelink/files/prelink.cron               | 61 ------------------
 sys-devel/prelink/files/prelink.service            |  6 --
 sys-devel/prelink/files/prelink.timer              | 10 ---
 sys-devel/prelink/metadata.xml                     |  8 ---
 sys-devel/prelink/prelink-20151030-r1.ebuild       | 72 ----------------------
 sys-devel/prelink/prelink-99999999.ebuild          | 71 ---------------------
 11 files changed, 374 deletions(-)