linux-kernel - Re: [PATCH -tip] ftrace: Correctly calculate the first function in the .text section

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.0907231043330.20997@gandalf.stny.rr.com>
Date:	Thu, 23 Jul 2009 10:50:03 -0400 (EDT)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Matt Fleming <matt@...sole-pimps.org>
cc:	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -tip] ftrace: Correctly calculate the first function in
 the .text section

On Thu, 23 Jul 2009, Matt Fleming wrote:

> This patch fixes a bug whereby recordmcount.pl didn't stop searching
> once it had correctly detected the function at the beginning of the
> .text section. To stop it searching, I needed to reset $read_function.
> The effect of this bug was that some entries in __mcount_loc section
> were created with the negative reloc addends. The last text section
> found was used as the base and all mcount calls were at a relative
> offset to that, so at final link time the addresses were fixed up to
> point to somewhere completely bogus.
> 
> This all resulted in ftrace dynamically modifying addresses that weren't
> actually mcount callsites.
> 
> I also noticed another bug and fixed up the condition from,

Famous quote: "When the comment does not match the code, they
are most likely both wrong".

> 
> 	if (!defined($ref_func) || !defined($weak{$text})) {
> to
> 	if (!defined($ref_func) && !defined($weak{$text})) {
> 
> This now matches the comment above the conditional.

This is the true bug. But the previous is not.

> 
> Signed-off-by: Matt Fleming <matt@...sole-pimps.org>
> ---
>  scripts/recordmcount.pl |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 7109e2b..356ff6a 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -414,8 +414,9 @@ while (<IN>) {
>  	    $read_function = 0;
>  	} else {
>  	    # if we already have a function, and this is weak, skip it
> -	    if (!defined($ref_func) || !defined($weak{$text})) {
> +	    if (!defined($ref_func) && !defined($weak{$text})) {

Ack on the above change.

>  		$ref_func = $text;
> +		$read_function = 0;

Nope, this is wrong. The idea is that we are looking for a function that 
is not local. Here's the full condition (with the applied fix):

        if (!defined($locals{$text}) && !defined($weak{$text})) {
            $ref_func = $text;
            $read_function = 0;
        } else {
            # if we already have a function, and this is weak, skip it
            if (!defined($ref_func) && !defined($weak{$text})) {
                $ref_func = $text;
            }
        }

Ideally, we want a non local function to use. If the first function in the 
section is local (defined as static) we record it, but we continue to look 
for more functions (don't set $read_function to 0). The first function in 
the section can easily be a static function, and there my be a global one 
later. If we find a global one later, we rather use that one. By setting 
$read_function to zero here, we stop our search, and we will be using a 
local static function instead.

When ever we use a static function as are reference point, we need to go 
through the pain of converting it to a global function to link in our 
mcount section, and then converting it back to local after it is linked. 

Offsets should be fine when negative, what issues do you see? I will admit 
though, that the s/||/&&/ was a real bug.

Thanks,

-- Steve

>  	    }
>  	}
>      } elsif ($read_headers && /$mcount_section/) {
> -- 
> 1.6.4.rc0
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/