linux-kernel - Re: [PATCH 1/5] perf: Validate address in symbol__inc_addr

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: <20120327141523.GA6931@infradead.org>
Date:	Tue, 27 Mar 2012 11:15:24 -0300
From:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
To:	David Miller <davem@...emloft.net>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Stephane Eranian <eranian@...gle.com>,
	Sorin Dumitru <dumitru.sorin87@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] perf: Validate address in symbol__inc_addr_samples().

Em Sun, Mar 25, 2012 at 04:28:08PM -0400, David Miller escreveu:
> If addr < sym->start then we are going to access past the end of the
> h->addr[] array, since the offset calculated will be huge.
> 
> Trigger an assertion instead of randomly scribbling memory when this
> situation occurs.

I'm applying this one instead, please check if you have any concerns
with this approach:

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index e5a462f..21c7590 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -64,8 +64,8 @@ int symbol__inc_addr_samples(struct symbol *sym,
struct map *map,
 
        pr_debug3("%s: addr=%#" PRIx64 "\n", __func__,
map->unmap_ip(map, addr));
 
-       if (addr >= sym->end)
-               return 0;
+       if (addr < sym->start || addr >= sym->end)
+               return -ERANGE;
 
        offset = addr - sym->start;
        h = annotation__histogram(notes, evidx);

And will make 'top' check the result, as 'report' already does, but
telling the user that there is a bug that should be reported to lkml,
using a WARN_ON_ONCE like method, telling the map, dso and symbol names.

This code already checked if the addr was after the end, but was
silently dropping samples in this case, oops.

So do a range check and return -ERANGE, so that tools can use their UI
specific way of telling the user that some samples are being dropped,
but don't panic and stop the tool.

I'll add Reported-by for Stephane and others that also reported this in
the past.


- Arnaldo
 
> Signed-off-by: David S. Miller <davem@...emloft.net>
> ---
>  tools/perf/util/annotate.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index e5a462f..734a503 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -15,6 +15,7 @@
>  #include "debug.h"
>  #include "annotate.h"
>  #include <pthread.h>
> +#include <assert.h>
>  
>  const char 	*disassembler_style;
>  
> @@ -64,6 +65,7 @@ int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>  
>  	pr_debug3("%s: addr=%#" PRIx64 "\n", __func__, map->unmap_ip(map, addr));
>  
> +	assert(addr >= sym->start);
>  	if (addr >= sym->end)
>  		return 0;
>  
> -- 
> 1.7.9.1
--
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/