LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Fix off by one in tools/perf strncpy size argument
@ 2020-03-09 10:48 Dominik 'disconnect3d' Czarnota
  2020-03-09 12:39 ` Arnaldo Carvalho de Melo
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Dominik 'disconnect3d' Czarnota @ 2020-03-09 10:48 UTC (permalink / raw)
  Cc: dominik.b.czarnota, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, John Keeping, Changbin Du,
	linux-kernel

From: disconnect3d <dominik.b.czarnota@gmail.com>

This patch fixes an off-by-one error in strncpy size argument in
tools/perf/util/map.c. The issue is that in:

        strncmp(filename, "/system/lib/", 11)

the passed string literal: "/system/lib/" has 12 bytes (without the NULL
byte) and the passed size argument is 11. As a result, the logic won't
match the ending "/" byte and will pass filepaths that are stored in
other directories e.g. "/system/libmalicious/bin" or just
"/system/libmalicious".

This functionality seems to be present only on Android. I assume the
/system/ directory is only writable by the root user, so I don't
think this bug has much (or any) security impact.

Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
---

Notes:
    I can't test this patch, so if someone can, please, do so.
    
    The bug could also be fixed by changing the size argument to `sizeof("string literal")-1` but I am not proposing this change as that would have to be changed in other places.
    
    There are also more cases like this in kernel sources which I am going to report soon.
    
    Also please note that other path comparisons in this file lack the "/" at the end and it seems they may imply similar issue. I haven't analysed the code deeply to see if that is a real issue.
    
    This bug has been found by running a massive grep-like search using Google's BigQuery on GitHub repositories data. I am also going to work on a CodeQL/Semmle query to be able to find more sophisticated cases like this that can't be found via grepping.

 tools/perf/util/map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a08ca276098e..addd7edb0486 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -89,7 +89,7 @@ static inline bool replace_android_lib(const char *filename, char *newfilename)
 		return true;
 	}
 
-	if (!strncmp(filename, "/system/lib/", 11)) {
+	if (!strncmp(filename, "/system/lib/", 12)) {
 		char *ndk, *app;
 		const char *arch;
 		size_t ndk_length;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix off by one in tools/perf strncpy size argument
  2020-03-09 10:48 [PATCH] Fix off by one in tools/perf strncpy size argument Dominik 'disconnect3d' Czarnota
@ 2020-03-09 12:39 ` Arnaldo Carvalho de Melo
  2020-03-09 12:48   ` Arnaldo Carvalho de Melo
  2020-03-19 14:04 ` [tip: perf/urgent] perf map: Fix off by one in strncpy() " tip-bot2 for disconnect3d
  2020-03-19 14:10 ` [tip: perf/core] " tip-bot2 for disconnect3d
  2 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-03-09 12:39 UTC (permalink / raw)
  To: Dominik 'disconnect3d' Czarnota
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, John Keeping, Changbin Du,
	linux-kernel, Michael Lentine, Stephane Eranian

Em Mon, Mar 09, 2020 at 11:48:53AM +0100, Dominik 'disconnect3d' Czarnota escreveu:
> From: disconnect3d <dominik.b.czarnota@gmail.com>
> 
> This patch fixes an off-by-one error in strncpy size argument in
> tools/perf/util/map.c. The issue is that in:
> 
>         strncmp(filename, "/system/lib/", 11)
> 
> the passed string literal: "/system/lib/" has 12 bytes (without the NULL
> byte) and the passed size argument is 11. As a result, the logic won't
> match the ending "/" byte and will pass filepaths that are stored in
> other directories e.g. "/system/libmalicious/bin" or just
> "/system/libmalicious".
> 
> This functionality seems to be present only on Android. I assume the
> /system/ directory is only writable by the root user, so I don't
> think this bug has much (or any) security impact.
> 
> Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
> ---
> 
> Notes:
>     I can't test this patch, so if someone can, please, do so.
>     
>     The bug could also be fixed by changing the size argument to `sizeof("string literal")-1` but I am not proposing this change as that would have to be changed in other places.

So, there are parts of this tools/perf/util/map.c that uses the idiom
you mention, for instance:

static inline int is_anon_memory(const char *filename, u32 flags)
{
        return flags & MAP_HUGETLB ||
               !strcmp(filename, "//anon") ||
               !strncmp(filename, "/dev/zero", sizeof("/dev/zero") - 1) ||
               !strncmp(filename, "/anon_hugepage", sizeof("/anon_hugepage") - 1);
}

So I think we should make all cases use this idim to avoid these
problems.

So I'll add your patch, then another, on top, that fixes the other
off-by-one errors introduced by the android specific code in this patch:

Fixes: eca818369996 ("perf tools: Add automatic remapping of Android libraries")

Put this in perf/urgent and then in perf/core move to the more robust
idiom,

Thanks,

- Arnaldo
     
>     There are also more cases like this in kernel sources which I am going to report soon.
>     
>     Also please note that other path comparisons in this file lack the "/" at the end and it seems they may imply similar issue. I haven't analysed the code deeply to see if that is a real issue.
>     
>     This bug has been found by running a massive grep-like search using Google's BigQuery on GitHub repositories data. I am also going to work on a CodeQL/Semmle query to be able to find more sophisticated cases like this that can't be found via grepping.
> 
>  tools/perf/util/map.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index a08ca276098e..addd7edb0486 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -89,7 +89,7 @@ static inline bool replace_android_lib(const char *filename, char *newfilename)
>  		return true;
>  	}
>  
> -	if (!strncmp(filename, "/system/lib/", 11)) {
> +	if (!strncmp(filename, "/system/lib/", 12)) {
>  		char *ndk, *app;
>  		const char *arch;
>  		size_t ndk_length;
> -- 
> 2.25.1
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Fix off by one in tools/perf strncpy size argument
  2020-03-09 12:39 ` Arnaldo Carvalho de Melo
@ 2020-03-09 12:48   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-03-09 12:48 UTC (permalink / raw)
  To: Dominik 'disconnect3d' Czarnota
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Song Liu, John Keeping, Changbin Du,
	linux-kernel, Michael Lentine, Stephane Eranian

Em Mon, Mar 09, 2020 at 09:39:40AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Mar 09, 2020 at 11:48:53AM +0100, Dominik 'disconnect3d' Czarnota escreveu:
> > From: disconnect3d <dominik.b.czarnota@gmail.com>
> > 
> > This patch fixes an off-by-one error in strncpy size argument in
> > tools/perf/util/map.c. The issue is that in:
> > 
> >         strncmp(filename, "/system/lib/", 11)
> > 
> > the passed string literal: "/system/lib/" has 12 bytes (without the NULL
> > byte) and the passed size argument is 11. As a result, the logic won't
> > match the ending "/" byte and will pass filepaths that are stored in
> > other directories e.g. "/system/libmalicious/bin" or just
> > "/system/libmalicious".
> > 
> > This functionality seems to be present only on Android. I assume the
> > /system/ directory is only writable by the root user, so I don't
> > think this bug has much (or any) security impact.
> > 
> > Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
> > ---
> > 
> > Notes:
> >     I can't test this patch, so if someone can, please, do so.
> >     
> >     The bug could also be fixed by changing the size argument to `sizeof("string literal")-1` but I am not proposing this change as that would have to be changed in other places.
> 
> So, there are parts of this tools/perf/util/map.c that uses the idiom
> you mention, for instance:
> 
> static inline int is_anon_memory(const char *filename, u32 flags)
> {
>         return flags & MAP_HUGETLB ||
>                !strcmp(filename, "//anon") ||
>                !strncmp(filename, "/dev/zero", sizeof("/dev/zero") - 1) ||
>                !strncmp(filename, "/anon_hugepage", sizeof("/anon_hugepage") - 1);
> }
> 
> So I think we should make all cases use this idim to avoid these
> problems.
> 
> So I'll add your patch, then another, on top, that fixes the other
> off-by-one errors introduced by the android specific code in this patch:

This is the only such off-by-one in that file, and I remembered we have
strstarts(), that we adopted from the kernel sources, so I think its a
better fit, no?

[acme@five linux]$ find . -name "*.c" | grep -v tools/ | xargs grep -w strstarts | wc -l
55
[acme@five linux]$ find tools/ -name "*.c" | xargs grep -w strstarts | wc -l
40
[acme@five linux]$
 
> Fixes: eca818369996 ("perf tools: Add automatic remapping of Android libraries")
> 
> Put this in perf/urgent and then in perf/core move to the more robust
> idiom,
> 
> Thanks,
> 
> - Arnaldo
>      
> >     There are also more cases like this in kernel sources which I am going to report soon.
> >     
> >     Also please note that other path comparisons in this file lack the "/" at the end and it seems they may imply similar issue. I haven't analysed the code deeply to see if that is a real issue.
> >     
> >     This bug has been found by running a massive grep-like search using Google's BigQuery on GitHub repositories data. I am also going to work on a CodeQL/Semmle query to be able to find more sophisticated cases like this that can't be found via grepping.
> > 
> >  tools/perf/util/map.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index a08ca276098e..addd7edb0486 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -89,7 +89,7 @@ static inline bool replace_android_lib(const char *filename, char *newfilename)
> >  		return true;
> >  	}
> >  
> > -	if (!strncmp(filename, "/system/lib/", 11)) {
> > +	if (!strncmp(filename, "/system/lib/", 12)) {
> >  		char *ndk, *app;
> >  		const char *arch;
> >  		size_t ndk_length;
> > -- 
> > 2.25.1
> > 
> 
> -- 
> 
> - Arnaldo

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip: perf/urgent] perf map: Fix off by one in strncpy() size argument
  2020-03-09 10:48 [PATCH] Fix off by one in tools/perf strncpy size argument Dominik 'disconnect3d' Czarnota
  2020-03-09 12:39 ` Arnaldo Carvalho de Melo
@ 2020-03-19 14:04 ` tip-bot2 for disconnect3d
  2020-03-19 14:10 ` [tip: perf/core] " tip-bot2 for disconnect3d
  2 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for disconnect3d @ 2020-03-19 14:04 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: disconnect3d, Alexander Shishkin, Changbin Du, Jiri Olsa,
	John Keeping, Mark Rutland, Michael Lentine, Namhyung Kim,
	Peter Zijlstra, Song Liu, Stephane Eranian,
	Arnaldo Carvalho de Melo, x86, LKML

The following commit has been merged into the perf/urgent branch of tip:

Commit-ID:     db2c549407d4a76563c579e4768f7d6d32afefba
Gitweb:        https://git.kernel.org/tip/db2c549407d4a76563c579e4768f7d6d32afefba
Author:        disconnect3d <dominik.b.czarnota@gmail.com>
AuthorDate:    Mon, 09 Mar 2020 11:48:53 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Mon, 09 Mar 2020 09:34:45 -03:00

perf map: Fix off by one in strncpy() size argument

This patch fixes an off-by-one error in strncpy size argument in
tools/perf/util/map.c. The issue is that in:

        strncmp(filename, "/system/lib/", 11)

the passed string literal: "/system/lib/" has 12 bytes (without the NULL
byte) and the passed size argument is 11. As a result, the logic won't
match the ending "/" byte and will pass filepaths that are stored in
other directories e.g. "/system/libmalicious/bin" or just
"/system/libmalicious".

This functionality seems to be present only on Android. I assume the
/system/ directory is only writable by the root user, so I don't think
this bug has much (or any) security impact.

Fixes: eca818369996 ("perf tools: Add automatic remapping of Android libraries")
Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Keeping <john@metanate.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Lentine <mlentine@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lore.kernel.org/lkml/20200309104855.3775-1-dominik.b.czarnota@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 9542851..b342f74 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -89,7 +89,7 @@ static inline bool replace_android_lib(const char *filename, char *newfilename)
 		return true;
 	}
 
-	if (!strncmp(filename, "/system/lib/", 11)) {
+	if (!strncmp(filename, "/system/lib/", 12)) {
 		char *ndk, *app;
 		const char *arch;
 		size_t ndk_length;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tip: perf/core] perf map: Fix off by one in strncpy() size argument
  2020-03-09 10:48 [PATCH] Fix off by one in tools/perf strncpy size argument Dominik 'disconnect3d' Czarnota
  2020-03-09 12:39 ` Arnaldo Carvalho de Melo
  2020-03-19 14:04 ` [tip: perf/urgent] perf map: Fix off by one in strncpy() " tip-bot2 for disconnect3d
@ 2020-03-19 14:10 ` tip-bot2 for disconnect3d
  2 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for disconnect3d @ 2020-03-19 14:10 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: disconnect3d, Alexander Shishkin, Changbin Du, Jiri Olsa,
	John Keeping, Mark Rutland, Michael Lentine, Namhyung Kim,
	Peter Zijlstra, Song Liu, Stephane Eranian,
	Arnaldo Carvalho de Melo, x86, LKML

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     b8fdcfb5a17f1d4fd503caef5c457005a765ecd7
Gitweb:        https://git.kernel.org/tip/b8fdcfb5a17f1d4fd503caef5c457005a765ecd7
Author:        disconnect3d <dominik.b.czarnota@gmail.com>
AuthorDate:    Mon, 09 Mar 2020 11:48:53 +01:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 11 Mar 2020 10:48:44 -03:00

perf map: Fix off by one in strncpy() size argument

This patch fixes an off-by-one error in strncpy size argument in
tools/perf/util/map.c. The issue is that in:

        strncmp(filename, "/system/lib/", 11)

the passed string literal: "/system/lib/" has 12 bytes (without the NULL
byte) and the passed size argument is 11. As a result, the logic won't
match the ending "/" byte and will pass filepaths that are stored in
other directories e.g. "/system/libmalicious/bin" or just
"/system/libmalicious".

This functionality seems to be present only on Android. I assume the
/system/ directory is only writable by the root user, so I don't think
this bug has much (or any) security impact.

Fixes: eca818369996 ("perf tools: Add automatic remapping of Android libraries")
Signed-off-by: disconnect3d <dominik.b.czarnota@gmail.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Keeping <john@metanate.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Lentine <mlentine@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lore.kernel.org/lkml/20200309104855.3775-1-dominik.b.czarnota@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 9542851..b342f74 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -89,7 +89,7 @@ static inline bool replace_android_lib(const char *filename, char *newfilename)
 		return true;
 	}
 
-	if (!strncmp(filename, "/system/lib/", 11)) {
+	if (!strncmp(filename, "/system/lib/", 12)) {
 		char *ndk, *app;
 		const char *arch;
 		size_t ndk_length;

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-03-19 14:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-09 10:48 [PATCH] Fix off by one in tools/perf strncpy size argument Dominik 'disconnect3d' Czarnota
2020-03-09 12:39 ` Arnaldo Carvalho de Melo
2020-03-09 12:48   ` Arnaldo Carvalho de Melo
2020-03-19 14:04 ` [tip: perf/urgent] perf map: Fix off by one in strncpy() " tip-bot2 for disconnect3d
2020-03-19 14:10 ` [tip: perf/core] " tip-bot2 for disconnect3d

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).