* [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name
@ 2018-11-01 18:27 Cyrill Gorcunov
2018-11-09 9:18 ` Thomas Gleixner
0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov @ 2018-11-01 18:27 UTC (permalink / raw)
To: LKML; +Cc: Andrey Vagin, Thomas Gleixner
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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name
2018-11-01 18:27 [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name Cyrill Gorcunov
@ 2018-11-09 9:18 ` Thomas Gleixner
2018-11-09 9:28 ` Cyrill Gorcunov
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2018-11-09 9:18 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: LKML, Andrey Vagin, Peter Zijlstra, Ingo Molnar
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name
2018-11-09 9:18 ` Thomas Gleixner
@ 2018-11-09 9:28 ` Cyrill Gorcunov
2018-11-09 11:31 ` [PATCH v2] fs/proc: timers -- Test for potential index overflow Cyrill Gorcunov
0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov @ 2018-11-09 9:28 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, Andrey Vagin, Peter Zijlstra, Ingo Molnar
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] fs/proc: timers -- Test for potential index overflow
2018-11-09 9:28 ` Cyrill Gorcunov
@ 2018-11-09 11:31 ` Cyrill Gorcunov
2018-12-21 20:28 ` Cyrill Gorcunov
0 siblings, 1 reply; 5+ messages in thread
From: Cyrill Gorcunov @ 2018-11-09 11:31 UTC (permalink / raw)
To: LKML; +Cc: Thomas Gleixner, Andrey Vagin, Peter Zijlstra, Ingo Molnar
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] fs/proc: timers -- Test for potential index overflow
2018-11-09 11:31 ` [PATCH v2] fs/proc: timers -- Test for potential index overflow Cyrill Gorcunov
@ 2018-12-21 20:28 ` Cyrill Gorcunov
0 siblings, 0 replies; 5+ messages in thread
From: Cyrill Gorcunov @ 2018-12-21 20:28 UTC (permalink / raw)
To: LKML; +Cc: Thomas Gleixner, Andrey Vagin, Peter Zijlstra, Ingo Molnar
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-21 20:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-01 18:27 [PATCH] posix-timers: Use array safe helper when fetching notification symbolic name Cyrill Gorcunov
2018-11-09 9:18 ` Thomas Gleixner
2018-11-09 9:28 ` Cyrill Gorcunov
2018-11-09 11:31 ` [PATCH v2] fs/proc: timers -- Test for potential index overflow Cyrill Gorcunov
2018-12-21 20:28 ` Cyrill Gorcunov
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).