[PATCH 0/4] rt-tests: deadline: Fix segmentation faults

[PATCH 0/4] rt-tests: deadline: Fix segmentation faults

[Kurt Kanzenbach]

Hi,

the deadline test binaries may crash under certain circumstances.

That seems to be caused by stack buffer overflows. While here, I've noticed that
some memory wasn't free'd. So, added that as well.

Thanks,
Kurt

Kurt Kanzenbach (4):
  rt-tests: cyclicdeadline: fix segmentation fault on close
  rt-tests: cyclicdeadline: add missing free calls
  rt-tests: deadline_tests: fix stack buffer flow
  rt-tests: deadline_test: add missing frees

 src/sched_deadline/cyclicdeadline.c | 4 +++-
 src/sched_deadline/deadline_test.c  | 7 ++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH 1/4] rt-tests: cyclicdeadline: fix segmentation fault on close

[Kurt Kanzenbach]

The current code generates a segmentation fault in the last free() call.

  $ sudo ./cyclicdeadline
  Using all CPUS
  /sys/kernel/debug/sched_features: Success
  interval: 600:1000
    Tested at 5us of 600us
  deadline thread 2963
  thread[2963] runtime=600us deadline=1000us
  main thread 2962
  fail 2 0
  T: 0 ( 2963) I:1000 C:   1268 Min:      7 Act:   55 Avg:   56 Max:     256
  [1]    2961 segmentation fault  sudo ./cyclicdeadline

This is caused by a buffer overflow in setup_ftrace_marker(). The appended
string is 21 not 14 characters wide. Fix it by using strlen() like the other
function do.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 src/sched_deadline/cyclicdeadline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
index 08460107c464..303b5e96647a 100644
--- a/src/sched_deadline/cyclicdeadline.c
+++ b/src/sched_deadline/cyclicdeadline.c
@@ -283,7 +283,7 @@ static void setup_ftrace_marker(void)
 {
 	struct stat st;
 	const char *debugfs = find_debugfs();
-	char files[strlen(debugfs) + 14];
+	char files[strlen(debugfs) + strlen("/tracing/trace_marker") + 1];
 	int ret;
 
 	if (strlen(debugfs) == 0)
-- 
2.11.0

[PATCH 2/4] rt-tests: cyclicdeadline: add missing free calls

[Kurt Kanzenbach]

The threads and thread data structures are allocated using calloc(). Free them
as well.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 src/sched_deadline/cyclicdeadline.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
index 303b5e96647a..f4dd26a2dba2 100644
--- a/src/sched_deadline/cyclicdeadline.c
+++ b/src/sched_deadline/cyclicdeadline.c
@@ -1261,6 +1261,8 @@ int main (int argc, char **argv)
 		}
 	}
 
+	free(thread);
+	free(sched_data);
 	free(setcpu_buf);
 	return 0;
 }
-- 
2.11.0

[PATCH 3/4] rt-tests: deadline_tests: fix stack buffer flow

[Kurt Kanzenbach]

The appended string is actually longer than 14 characters. Use strlen() to
compute the correct length like the other functions do.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 src/sched_deadline/deadline_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
index b213c206559c..4826557d0624 100644
--- a/src/sched_deadline/deadline_test.c
+++ b/src/sched_deadline/deadline_test.c
@@ -435,7 +435,7 @@ static void setup_ftrace_marker(void)
 {
 	struct stat st;
 	const char *debugfs = find_debugfs();
-	char files[strlen(debugfs) + 14];
+	char files[strlen(debugfs) + strlen("/tracing/trace_marker") + 1];
 	int ret;
 
 	if (strlen(debugfs) == 0)
-- 
2.11.0

[PATCH 4/4] rt-tests: deadline_test: add missing frees

[Kurt Kanzenbach]

The threads and thread data structures are allocated using calloc(). But never
free'd. So, free them.

While here, remove the if statement for setcpu_buf. Free() is NULL safe.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 src/sched_deadline/deadline_test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
index 4826557d0624..69e6fd18b73b 100644
--- a/src/sched_deadline/deadline_test.c
+++ b/src/sched_deadline/deadline_test.c
@@ -2092,8 +2092,9 @@ int main (int argc, char **argv)
 		printf("\n");
 	}
 
-	if (!setcpu_buf)
-		free(setcpu_buf);
+	free(thread);
+	free(sched_data);
+	free(setcpu_buf);
 
 	return 0;
 }
-- 
2.11.0

Re: [PATCH 4/4] rt-tests: deadline_test: add missing frees

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> The threads and thread data structures are allocated using calloc(). But never
> free'd. So, free them.
> 
> While here, remove the if statement for setcpu_buf. Free() is NULL safe.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  src/sched_deadline/deadline_test.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> index 4826557d0624..69e6fd18b73b 100644
> --- a/src/sched_deadline/deadline_test.c
> +++ b/src/sched_deadline/deadline_test.c
> @@ -2092,8 +2092,9 @@ int main (int argc, char **argv)
>  		printf("\n");
>  	}
>  
> -	if (!setcpu_buf)
> -		free(setcpu_buf);
> +	free(thread);
> +	free(sched_data);
> +	free(setcpu_buf);
>  
>  	return 0;
>  }
> -- 
> 2.11.0

Does this remove your own if statement that I didn't even apply yet? :)

Maybe, you want to regenerate this patch without that?

Thanks

John

Re: [PATCH 4/4] rt-tests: deadline_test: add missing frees

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Hi,

On Thu, Apr 04, 2019 at 05:13:51PM +0200, John Kacur wrote:
>
>
> On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:
>
> > The threads and thread data structures are allocated using calloc(). But never
> > free'd. So, free them.
> >
> > While here, remove the if statement for setcpu_buf. Free() is NULL safe.
> >
> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > ---
> >  src/sched_deadline/deadline_test.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> > index 4826557d0624..69e6fd18b73b 100644
> > --- a/src/sched_deadline/deadline_test.c
> > +++ b/src/sched_deadline/deadline_test.c
> > @@ -2092,8 +2092,9 @@ int main (int argc, char **argv)
> >  		printf("\n");
> >  	}
> >
> > -	if (!setcpu_buf)
> > -		free(setcpu_buf);
> > +	free(thread);
> > +	free(sched_data);
> > +	free(setcpu_buf);
> >
> >  	return 0;
> >  }
> > --
> > 2.11.0
>
> Does this remove your own if statement that I didn't even apply yet?
> :)

No. That's a different binary. My patch was made against cyclicdeadline.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] rt-tests: deadline_test: add missing frees

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> Hi,
> 
> On Thu, Apr 04, 2019 at 05:13:51PM +0200, John Kacur wrote:
> >
> >
> > On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:
> >
> > > The threads and thread data structures are allocated using calloc(). But never
> > > free'd. So, free them.
> > >
> > > While here, remove the if statement for setcpu_buf. Free() is NULL safe.
> > >
> > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> > > ---
> > >  src/sched_deadline/deadline_test.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> > > index 4826557d0624..69e6fd18b73b 100644
> > > --- a/src/sched_deadline/deadline_test.c
> > > +++ b/src/sched_deadline/deadline_test.c
> > > @@ -2092,8 +2092,9 @@ int main (int argc, char **argv)
> > >  		printf("\n");
> > >  	}
> > >
> > > -	if (!setcpu_buf)
> > > -		free(setcpu_buf);
> > > +	free(thread);
> > > +	free(sched_data);
> > > +	free(setcpu_buf);
> > >
> > >  	return 0;
> > >  }
> > > --
> > > 2.11.0
> >
> > Does this remove your own if statement that I didn't even apply yet?
> > :)
> 
> No. That's a different binary. My patch was made against cyclicdeadline.
> 
> Thanks,
> Kurt

Ok, got it, thanks!

John

Re: [PATCH 1/4] rt-tests: cyclicdeadline: fix segmentation fault on close

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> The current code generates a segmentation fault in the last free() call.
> 
>   $ sudo ./cyclicdeadline
>   Using all CPUS
>   /sys/kernel/debug/sched_features: Success
>   interval: 600:1000
>     Tested at 5us of 600us
>   deadline thread 2963
>   thread[2963] runtime=600us deadline=1000us
>   main thread 2962
>   fail 2 0
>   T: 0 ( 2963) I:1000 C:   1268 Min:      7 Act:   55 Avg:   56 Max:     256
>   [1]    2961 segmentation fault  sudo ./cyclicdeadline
> 
> This is caused by a buffer overflow in setup_ftrace_marker(). The appended
> string is 21 not 14 characters wide. Fix it by using strlen() like the other
> function do.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  src/sched_deadline/cyclicdeadline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index 08460107c464..303b5e96647a 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -283,7 +283,7 @@ static void setup_ftrace_marker(void)
>  {
>  	struct stat st;
>  	const char *debugfs = find_debugfs();
> -	char files[strlen(debugfs) + 14];
> +	char files[strlen(debugfs) + strlen("/tracing/trace_marker") + 1];
>  	int ret;
>  
>  	if (strlen(debugfs) == 0)
> -- 
> 2.11.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 2/4] rt-tests: cyclicdeadline: add missing free calls

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> The threads and thread data structures are allocated using calloc(). Free them
> as well.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  src/sched_deadline/cyclicdeadline.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index 303b5e96647a..f4dd26a2dba2 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -1261,6 +1261,8 @@ int main (int argc, char **argv)
>  		}
>  	}
>  
> +	free(thread);
> +	free(sched_data);
>  	free(setcpu_buf);
>  	return 0;
>  }
> -- 
> 2.11.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 3/4] rt-tests: deadline_tests: fix stack buffer flow

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> The appended string is actually longer than 14 characters. Use strlen() to
> compute the correct length like the other functions do.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  src/sched_deadline/deadline_test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> index b213c206559c..4826557d0624 100644
> --- a/src/sched_deadline/deadline_test.c
> +++ b/src/sched_deadline/deadline_test.c
> @@ -435,7 +435,7 @@ static void setup_ftrace_marker(void)
>  {
>  	struct stat st;
>  	const char *debugfs = find_debugfs();
> -	char files[strlen(debugfs) + 14];
> +	char files[strlen(debugfs) + strlen("/tracing/trace_marker") + 1];
>  	int ret;
>  
>  	if (strlen(debugfs) == 0)
> -- 
> 2.11.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 4/4] rt-tests: deadline_test: add missing frees

[John Kacur]

On Thu, 4 Apr 2019, Kurt Kanzenbach wrote:

> The threads and thread data structures are allocated using calloc(). But never
> free'd. So, free them.
> 
> While here, remove the if statement for setcpu_buf. Free() is NULL safe.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  src/sched_deadline/deadline_test.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> index 4826557d0624..69e6fd18b73b 100644
> --- a/src/sched_deadline/deadline_test.c
> +++ b/src/sched_deadline/deadline_test.c
> @@ -2092,8 +2092,9 @@ int main (int argc, char **argv)
>  		printf("\n");
>  	}
>  
> -	if (!setcpu_buf)
> -		free(setcpu_buf);
> +	free(thread);
> +	free(sched_data);
> +	free(setcpu_buf);
>  
>  	return 0;
>  }
> -- 
> 2.11.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>