[PATCH] posix-timers: Use array safe helper when fetching notification symbolic name

[PATCH] posix-timers: Use array safe helper when fetching notification symbolic name

[Cyrill Gorcunov]

When showing timer's notify symbolic name make sure we never fetch a value
sitting outside of the names array. Though the former issue displaying
timer->it_sigev_notify has been fixed by Thomas in commit cef31d9af9082434,
better to make sure we won't hit it again in furher modifications.

Cc: Andrey Vagin <avagin@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 fs/proc/base.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-ml.git/fs/proc/base.c
===================================================================
--- linux-ml.git.orig/fs/proc/base.c
+++ linux-ml.git/fs/proc/base.c
@@ -2287,7 +2287,7 @@ static int show_timer(struct seq_file *m
 {
 	struct k_itimer *timer;
 	struct timers_private *tp = m->private;
-	int notify;
+	int notify, nidx;
 	static const char * const nstr[] = {
 		[SIGEV_SIGNAL] = "signal",
 		[SIGEV_NONE] = "none",
@@ -2296,13 +2296,13 @@ static int show_timer(struct seq_file *m
 
 	timer = list_entry((struct list_head *)v, struct k_itimer, list);
 	notify = timer->it_sigev_notify;
+	nidx = array_index_nospec(notify & ~SIGEV_THREAD_ID, ARRAY_SIZE(nstr));
 
 	seq_printf(m, "ID: %d\n", timer->it_id);
 	seq_printf(m, "signal: %d/%px\n",
 		   timer->sigq->info.si_signo,
 		   timer->sigq->info.si_value.sival_ptr);
-	seq_printf(m, "notify: %s/%s.%d\n",
-		   nstr[notify & ~SIGEV_THREAD_ID],
+	seq_printf(m, "notify: %s/%s.%d\n", nstr[nidx],
 		   (notify & SIGEV_THREAD_ID) ? "tid" : "pid",
 		   pid_nr_ns(timer->it_pid, tp->ns));
 	seq_printf(m, "ClockID: %d\n", timer->it_clock);

Re: [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name

[Thomas Gleixner]

On Thu, 1 Nov 2018, Cyrill Gorcunov wrote:

> When showing timer's notify symbolic name make sure we never fetch a value
> sitting outside of the names array. Though the former issue displaying
> timer->it_sigev_notify has been fixed by Thomas in commit cef31d9af9082434,
> better to make sure we won't hit it again in furher modifications.
> 
> Cc: Andrey Vagin <avagin@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  fs/proc/base.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-ml.git/fs/proc/base.c
> ===================================================================
> --- linux-ml.git.orig/fs/proc/base.c
> +++ linux-ml.git/fs/proc/base.c
> @@ -2287,7 +2287,7 @@ static int show_timer(struct seq_file *m
>  {
>  	struct k_itimer *timer;
>  	struct timers_private *tp = m->private;
> -	int notify;
> +	int notify, nidx;
>  	static const char * const nstr[] = {
>  		[SIGEV_SIGNAL] = "signal",
>  		[SIGEV_NONE] = "none",
> @@ -2296,13 +2296,13 @@ static int show_timer(struct seq_file *m
>  
>  	timer = list_entry((struct list_head *)v, struct k_itimer, list);
>  	notify = timer->it_sigev_notify;
> +	nidx = array_index_nospec(notify & ~SIGEV_THREAD_ID, ARRAY_SIZE(nstr));

I completely understand your intention, but this is misleading. The above
is really not a speculation gadget.

I'd rather do an open coded check here and fail the thing instead of
printing wrong information:

    	   nidx = timer->it_sigev_notify & ~SIGEV_THREAD_ID;
	   if (nidx >= ARRAY_SIZE(nstr))
	   	return -EINVAL;

Thanks,

	tglx

Re: [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name

[Cyrill Gorcunov]

On Fri, Nov 09, 2018 at 10:18:25AM +0100, Thomas Gleixner wrote:
> > +	nidx = array_index_nospec(notify & ~SIGEV_THREAD_ID, ARRAY_SIZE(nstr));
> 
> I completely understand your intention, but this is misleading. The above
> is really not a speculation gadget.
> 
> I'd rather do an open coded check here and fail the thing instead of
> printing wrong information:
> 
>     	   nidx = timer->it_sigev_notify & ~SIGEV_THREAD_ID;
> 	   if (nidx >= ARRAY_SIZE(nstr))
> 	   	return -EINVAL;

Yes, I thought about such approach and did the similar code
in first place, then I dropped it because didn't want additional
"if"s here (your fix in the former commit already does all verification
needed). But thinking more I agree: this code is not time critical
and spending a few cycles won't hurt much. Will update, thanks Thomas!

[PATCH v2] fs/proc: timers -- Test for potential index overflow

[Cyrill Gorcunov]

When showing timer's notify symbolic name make sure we never fetch a value
sitting outside of the names array. Though the former issue displaying
timer->it_sigev_notify has been fixed by Thomas in commit cef31d9af9082434,
better to make sure we won't hit it again on furher modifications.

v2: Use explicit index overflow test (by tglx@). Since
    it should never happen add warn-once to notify.

Cc: Andrey Vagin <avagin@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 fs/proc/base.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-ml.git/fs/proc/base.c
===================================================================
--- linux-ml.git.orig/fs/proc/base.c
+++ linux-ml.git/fs/proc/base.c
@@ -2297,6 +2297,12 @@ static int show_timer(struct seq_file *m
 	timer = list_entry((struct list_head *)v, struct k_itimer, list);
 	notify = timer->it_sigev_notify;
 
+	if ((notify & ~SIGEV_THREAD_ID) >= ARRAY_SIZE(nstr)) {
+		WARN_ONCE(1, "timer: Bad signal notify mode %#x on ID %d\n",
+			  notify, timer->it_id);
+		return -EINVAL;
+	}
+
 	seq_printf(m, "ID: %d\n", timer->it_id);
 	seq_printf(m, "signal: %d/%px\n",
 		   timer->sigq->info.si_signo,

Re: [PATCH v2] fs/proc: timers -- Test for potential index overflow

[Cyrill Gorcunov]

On Fri, Nov 09, 2018 at 02:31:53PM +0300, Cyrill Gorcunov wrote:
> When showing timer's notify symbolic name make sure we never fetch a value
> sitting outside of the names array. Though the former issue displaying
> timer->it_sigev_notify has been fixed by Thomas in commit cef31d9af9082434,
> better to make sure we won't hit it again on furher modifications.
> 
> v2: Use explicit index overflow test (by tglx@). Since
>     it should never happen add warn-once to notify.
> 
> Cc: Andrey Vagin <avagin@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Seems the patch is lost due to other duties. Guys, should
I resend it or there some objections?