Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 520198 - pax-utils.eclass / paxctl fails to mark binaries which are too large
Summary: pax-utils.eclass / paxctl fails to mark binaries which are too large
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Hardened (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Rick Farina (Zero_Chaos)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-18 16:57 UTC by Rick Farina (Zero_Chaos)
Modified: 2014-09-03 04:27 UTC (History)
1 user (show)

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


Attachments
New proposed pax-utils.eclass. (pax-utils.eclass,6.62 KB, text/plain)
2014-08-18 18:33 UTC, Anthony Basile
Details
Fix indentation (pax-utils.eclass,6.61 KB, text/plain)
2014-08-18 18:35 UTC, Anthony Basile
Details
Correct logic wrt to failures (pax-utils.eclass,6.58 KB, text/plain)
2014-08-23 13:27 UTC, Anthony Basile
Details
big file patch for paxctl (big.patch,590 bytes, patch)
2014-09-02 13:37 UTC, Rick Farina (Zero_Chaos)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Farina (Zero_Chaos) gentoo-dev 2014-08-18 16:57:52 UTC
when building chromium with debug options the resulting chrome binary is huge:

ozzie Release # ls -alh chrome
-rwxr-xr-x 1 portage portage 2.3G Aug 18 12:55 chrome*

As such, paxctl fails to mark it:

ozzie Release # paxctl -m chrome
file chrome is too big

this leads to broken chrome.

Interestingly, both paxctl-ng and scanelf can mark it just fine, however the eclass is written so that if paxctl is found it is used, and if it fails, then just fail and don't try anything else.  It might be best to just cascade and try one after the other instead of only trying to mark with the first capable program found.
Comment 1 Rick Farina (Zero_Chaos) gentoo-dev 2014-08-18 17:15:36 UTC
appropriate snippit of build.log:

 * PT PaX marking -m with paxctl
 *      out/Release/chrome
 * Failed to set PT_PAX markings -m for:
 *      out/Release/chrome
Comment 2 Anthony Basile gentoo-dev 2014-08-18 17:16:15 UTC
The failing code is in paxctl.c where

  if (st.st_size < 0 || INT_MAX < st.st_size) {
    close(fd);
    if (!state->quiet)
      fprintf(stderr, "file %s is too big\n", state->argv[state->files]);
    return EXIT_FAILURE;
  }

On amd64, INT_MAX is 4G unsigned but st_size from struct stat is of type off_t which is 64 bits wide, so the check is wrong.
Comment 3 Anthony Basile gentoo-dev 2014-08-18 18:26:45 UTC
We probably want to make the pax-utils.eclass more robust too.  Right now it written with the assumption that one method will be used for *all* the files or else its a failure.  A better approach is to try each method on a file by file basis.  So we should change the eclass to be someething like

    if has PT ${PAX_MARKINGS}; then
        _pax_list_files einfo "$@"
        for f in "$@"; do

            #First try paxctl -> this might try to create/convert program headers
            if type -p paxctl > /dev/null; then
                einfo "PT PaX marking -${flags} ${f} with paxctl"
                    # First, try modifying the existing PAX_FLAGS header
                    paxctl -q${flags} "${f}" && continue
                    # Second, try creating a PT_PAX header (works on ET_EXEC)
                    # Even though this is less safe, most exes need it, eg bug #463170
                    paxctl -qC${flags} "${f}" && continue
                    # Third, try stealing the (unused under PaX) PT_GNU_STACK header
                    paxctl -qc${flags} "${f}" && continue

            #Next try paxctl-ng -> this will not create/convert any program headers
            elif type -p paxctl-ng > /dev/null && paxctl-ng -L ; then
                einfo "PT PaX marking -${flags} ${f} with paxctl-ng"
                flags="${flags//z}"
                [[ ${dodefault} == "yes" ]] && paxctl-ng -L -z "${f}"
                [[ "${flags}" ]] || continue
                paxctl-ng -L -${flags} "${f}" && continue

            #Finally fall back on scanelf
            elif type -p scanelf > /dev/null && [[ ${PAX_MARKINGS} != "none" ]]; then
                einfo "Fallback PaX marking -${flags} with scanelf"
                scanelf -Xxz ${flags} "$f"

            #We failed to set PT_PAX flags
            elif [[ ${PAX_MARKINGS} != "none" ]]; then
                elog "Failed to set PT_PAX markings -${flags} ${f} for:"
                _pax_list_files elog ${f}
                ret=1
            fi
        done
    fi


and similarly if has XT ${PAX_MARKINGS}.
Comment 4 Anthony Basile gentoo-dev 2014-08-18 18:33:21 UTC
Created attachment 383086 [details]
New proposed pax-utils.eclass.
Comment 5 Anthony Basile gentoo-dev 2014-08-18 18:35:39 UTC
Created attachment 383088 [details]
Fix indentation
Comment 6 Rick Farina (Zero_Chaos) gentoo-dev 2014-08-18 19:27:08 UTC
(In reply to Anthony Basile from comment #5)
> Created attachment 383088 [details]
> Fix indentation

This doesn't fix the original issue because the conditionals are wrong:

if type -p paxctl > /dev/null; then
...
elif type -p paxctl-ng > /dev/null && paxctl-ng -L ; then


This means that if paxctl is found, it won't hit the "else if" for paxctl-ng at all. It should be if not elif.
Comment 7 Anthony Basile gentoo-dev 2014-08-19 13:24:20 UTC
(In reply to Rick Farina (Zero_Chaos) from comment #6)
> (In reply to Anthony Basile from comment #5)
> > Created attachment 383088 [details]
> > Fix indentation
> 
> This doesn't fix the original issue because the conditionals are wrong:
> 
> if type -p paxctl > /dev/null; then
> ...
> elif type -p paxctl-ng > /dev/null && paxctl-ng -L ; then
> 
> 
> This means that if paxctl is found, it won't hit the "else if" for paxctl-ng
> at all. It should be if not elif.

Right.  Given the continue's changing the elif's to just ifs should fix that.  Let's keep reviewing before I upload the next version, in case there are more issues I'm missing.
Comment 8 Rick Farina (Zero_Chaos) gentoo-dev 2014-08-20 16:02:00 UTC
(In reply to Anthony Basile from comment #7)
> Right.  Given the continue's changing the elif's to just ifs should fix
> that.  Let's keep reviewing before I upload the next version, in case there
> are more issues I'm missing.

That was everything I saw, but more eyes certainly won't hurt.  C'mon hardened team et al, weigh in.
Comment 9 Anthony Basile gentoo-dev 2014-08-23 13:27:33 UTC
Created attachment 383452 [details]
Correct logic wrt to failures

@zero_chaos, please test if this works in you above failure with chrome and if it does, I'll email gentoo-dev@ for a final review before committing.
Comment 10 Rick Farina (Zero_Chaos) gentoo-dev 2014-08-26 12:47:31 UTC
 *      out/Release/chrome
 * PT PaX marking -m out/Release/chrome with paxctl
 * PT PaX marking -m out/Release/chrome with paxctl-ng

ozzie eclass # paxctl -v /usr/lib64/chromium-browser/chrome
PaX control v0.7
Copyright 2004,2005,2006,2007,2009,2010,2011,2012 PaX Team <pageexec@freemail.hu>

- PaX flags: -----m-x-e-- [/usr/lib64/chromium-browser/chrome]
	MPROTECT is disabled
	RANDEXEC is disabled
	EMUTRAMP is disabled


looks like this works great to me.
Comment 11 PaX Team 2014-08-29 22:24:34 UTC
(In reply to Anthony Basile from comment #2)
> The failing code is in paxctl.c where
> 
>   if (st.st_size < 0 || INT_MAX < st.st_size) {
>     close(fd);
>     if (!state->quiet)
>       fprintf(stderr, "file %s is too big\n", state->argv[state->files]);
>     return EXIT_FAILURE;
>   }
> 
> On amd64, INT_MAX is 4G unsigned but st_size from struct stat is of type
> off_t which is 64 bits wide, so the check is wrong.

INT_MAX is 2G(-1) everywhere in the gcc world since 'int' is a signed 32 bit type. the idea behind the check is to prevent the truncation on 32 bit archs when casting off_t to size_t later but i guess the limit should be SIZE_MAX instead of INT_MAX (at least when -D_FILE_OFFSET_BITS=64 but iirc _GNU_SOURCE enables that).
Comment 12 Anthony Basile gentoo-dev 2014-08-30 15:06:40 UTC
(In reply to Rick Farina (Zero_Chaos) from comment #10)
> looks like this works great to me.

Okay its on the tree with a few minor changes discussed on the list.  Please keep testing.
Comment 13 Anthony Basile gentoo-dev 2014-08-30 15:18:58 UTC
(In reply to PaX Team from comment #11)
> (In reply to Anthony Basile from comment #2)
> > The failing code is in paxctl.c where
> > 
> >   if (st.st_size < 0 || INT_MAX < st.st_size) {
> >     close(fd);
> >     if (!state->quiet)
> >       fprintf(stderr, "file %s is too big\n", state->argv[state->files]);
> >     return EXIT_FAILURE;
> >   }
> > 
> > On amd64, INT_MAX is 4G unsigned but st_size from struct stat is of type
> > off_t which is 64 bits wide, so the check is wrong.
> 
> INT_MAX is 2G(-1) everywhere in the gcc world since 'int' is a signed 32 bit
> type. the idea behind the check is to prevent the truncation on 32 bit archs
> when casting off_t to size_t later but i guess the limit should be SIZE_MAX
> instead of INT_MAX (at least when -D_FILE_OFFSET_BITS=64 but iirc
> _GNU_SOURCE enables that).

1) Ack on INT_MAX.

2) SIZE_MAX is simply defined in stdint.h as

/* Limit of `size_t' type.  */
# if __WORDSIZE == 64
#  define SIZE_MAX              (18446744073709551615UL)
# else
#  define SIZE_MAX              (4294967295U)
# endif

is this okay when on a 32-bit with -D_FILE_OFFSET_BITS=64 ?
Comment 14 PaX Team 2014-08-30 16:46:03 UTC
(In reply to Anthony Basile from comment #13)
> is this okay when on a 32-bit with -D_FILE_OFFSET_BITS=64 ?

i settled on LONG_MAX instead to avoid casting between signed/unsigned types and i guess noone will care about large file support on 32 bit archs anyway. can you give this patch a try?

--- a/paxctl.c
+++ b/paxctl.c
@@ -1,10 +1,11 @@
  */

 #define _GNU_SOURCE
+#define _FILE_OFFSET_BITS 64
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -121,7 +122,7 @@ static int pax_verify_file(struct pax_state * const state)
     return EXIT_FAILURE;
   }

-  if (st.st_size < 0 || INT_MAX < st.st_size) {
+  if (st.st_size < 0 || LONG_MAX < st.st_size) {
     close(fd);
     if (!state->quiet)
       fprintf(stderr, "file %s is too big\n", state->argv[state->files]);
Comment 15 Rick Farina (Zero_Chaos) gentoo-dev 2014-09-02 13:37:38 UTC
Created attachment 384118 [details, diff]
big file patch for paxctl

Please note, attaching patches to the bug is vastly preferred to inlining them, I have a below 0% success rate with inline patches.

That said, I manually applied it and it seems to work just fine. Thanks.
Comment 16 PaX Team 2014-09-02 13:54:17 UTC
(In reply to Rick Farina (Zero_Chaos) from comment #15)
> Please note, attaching patches to the bug is vastly preferred to inlining
> them, I have a below 0% success rate with inline patches.

i just wanted someone to try it quickly (it's a simple 2-liner after all), i'll of course put it into a real paxctl release next, thanks for testing it!
Comment 17 PaX Team 2014-09-02 18:09:21 UTC
paxctl 0.9 is out and should fix this problem on 64 bit archs.
Comment 18 Rick Farina (Zero_Chaos) gentoo-dev 2014-09-03 04:27:58 UTC
Some fairly non-trivial eclass improvements and a new release of paxctl with a bug fixed.  If only all bugs could be ended this quickly and properly.  Great work to all, and thanks.