All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tracing: Resolve stack corruption due to string copy
@ 2017-05-03 10:11 Amit Pundir
  2017-05-03 14:00 ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Amit Pundir @ 2017-05-03 10:11 UTC (permalink / raw)
  To: Steven Rostedt, Ingo Molnar; +Cc: Greg KH, lkml, Amey Telawane, stable

From: Amey Telawane <ameyt@codeaurora.org>

Strcpy has no limit on string being copied which causes
stack corruption leading to kernel panic. Use strlcpy to
resolve the issue by providing length of string to be copied.

Cc: stable@vger.kernel.org
Signed-off-by: Amey Telawane <ameyt@codeaurora.org>
[AmitP: Cherry-picked this commit from CodeAurora kernel/msm-3.10
https://source.codeaurora.org/quic/la/kernel/msm-3.10/commit/?id=2161ae9a70b12cf18ac8e5952a20161ffbccb477]
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
This patch featured in Android Security Bulletin for May 2017,
https://source.android.com/security/bulletin/2017-05-01#eop-in-kernel-trace-subsystem,
but it is not upstreamed yet and I couldn't find any previous
upstream submission as well.

 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index bd8fb5cfda4d..b227e141e1f1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])
 
 	map = savedcmd->map_pid_to_cmdline[pid];
 	if (map != NO_CMDLINE_MAP)
-		strcpy(comm, get_saved_cmdlines(map));
+		strlcpy(comm, get_saved_cmdlines(map), TASK_COMM_LEN - 1);
 	else
 		strcpy(comm, "<...>");
 }
-- 
2.7.4

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

* Re: [PATCH] tracing: Resolve stack corruption due to string copy
  2017-05-03 10:11 [PATCH] tracing: Resolve stack corruption due to string copy Amit Pundir
@ 2017-05-03 14:00 ` Steven Rostedt
  2017-05-03 14:09   ` [PATCH] tracing: Use strlcpy() instead of strcpy() in __trace_find_cmdline() (Was: Re: [PATCH] tracing: Resolve stack corruption due to string copy) Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2017-05-03 14:00 UTC (permalink / raw)
  To: Amit Pundir; +Cc: Ingo Molnar, Greg KH, lkml, Amey Telawane, stable

On Wed,  3 May 2017 15:41:14 +0530
Amit Pundir <amit.pundir@linaro.org> wrote:

> From: Amey Telawane <ameyt@codeaurora.org>
> 
> Strcpy has no limit on string being copied which causes
> stack corruption leading to kernel panic. Use strlcpy to
> resolve the issue by providing length of string to be copied.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Amey Telawane <ameyt@codeaurora.org>
> [AmitP: Cherry-picked this commit from CodeAurora kernel/msm-3.10
> https://source.codeaurora.org/quic/la/kernel/msm-3.10/commit/?id=2161ae9a70b12cf18ac8e5952a20161ffbccb477]
> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> ---
> This patch featured in Android Security Bulletin for May 2017,
> https://source.android.com/security/bulletin/2017-05-01#eop-in-kernel-trace-subsystem,
> but it is not upstreamed yet and I couldn't find any previous
> upstream submission as well.
> 
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index bd8fb5cfda4d..b227e141e1f1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])
>  
>  	map = savedcmd->map_pid_to_cmdline[pid];
>  	if (map != NO_CMDLINE_MAP)
> -		strcpy(comm, get_saved_cmdlines(map));
> +		strlcpy(comm, get_saved_cmdlines(map), TASK_COMM_LEN - 1);

Actually it should be TASK_COMM_LEN (without the -1), as all comms
passed in must be of that length. The strlcpy will add '\0' to the
len-1 passed in. Passing in TASK_COMM_LEN-1 will yield a '\0' at
TASK_COMM_LEN-2.

Note, I don't see anyway to trigger a bug. To me this looks simply like
someone saw "strcpy" and said to themselves "oh this is a bug", when
actuality it is not. I don't mind the extra security added, but I don't
think this even needs to go to stable. The reason is that the comm used
within the kernel is always created by the kernel, and always has a
terminating nul character. There's other places in the kernel that will
bug if that is not true.

The comm is set by __set_task_comm() which uses strlcpy().

I'll take this patch, but I'm going to strip the stable tag from it.

-- Steve

>  	else
>  		strcpy(comm, "<...>");
>  }

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

* [PATCH] tracing: Use strlcpy() instead of strcpy() in __trace_find_cmdline() (Was: Re: [PATCH] tracing: Resolve stack corruption due to string copy)
  2017-05-03 14:00 ` Steven Rostedt
@ 2017-05-03 14:09   ` Steven Rostedt
  0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2017-05-03 14:09 UTC (permalink / raw)
  To: Amit Pundir; +Cc: Ingo Molnar, Greg KH, lkml, Amey Telawane, stable

[ Here's the new patch ]

From: Amey Telawane <ameyt@codeaurora.org>

Strcpy is inherently not safe, and strlcpy() should be used instead.
__trace_find_cmdline() uses strcpy() because the comms saved must have a
terminating nul character, but it doesn't hurt to add the extra protection
of using strlcpy() instead of strcpy().

Link: http://lkml.kernel.org/r/1493806274-13936-1-git-send-email-amit.pundir@linaro.org

Signed-off-by: Amey Telawane <ameyt@codeaurora.org>
[AmitP: Cherry-picked this commit from CodeAurora kernel/msm-3.10
https://source.codeaurora.org/quic/la/kernel/msm-3.10/commit/?id=2161ae9a70b12cf18ac8e5952a20161ffbccb477]
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
[ Updated change log and removed the "- 1" from len parameter ]
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5b645b0..c1b0f53 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1976,7 +1976,7 @@ static void __trace_find_cmdline(int pid, char comm[])
 
 	map = savedcmd->map_pid_to_cmdline[pid];
 	if (map != NO_CMDLINE_MAP)
-		strcpy(comm, get_saved_cmdlines(map));
+		strlcpy(comm, get_saved_cmdlines(map), TASK_COMM_LEN);
 	else
 		strcpy(comm, "<...>");
 }
-- 
2.9.3

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

end of thread, other threads:[~2017-05-03 14:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-03 10:11 [PATCH] tracing: Resolve stack corruption due to string copy Amit Pundir
2017-05-03 14:00 ` Steven Rostedt
2017-05-03 14:09   ` [PATCH] tracing: Use strlcpy() instead of strcpy() in __trace_find_cmdline() (Was: Re: [PATCH] tracing: Resolve stack corruption due to string copy) Steven Rostedt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.