posix-timers: Use array safe helper when fetching notification symbolic name
PATCH 1/1 posix-timers: Use array safe helper when fetching notification symbolic name
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.
--- fs/proc/base.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
fs/proc/base.c +3 -3
@@ -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);
Quoted 26 lines · 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_notifyhas been fixed by Thomas in commitcef31d9af9082434, better to make sure we won't hit it again in furher modifications.Cc Andrey Vagin <[email protected]>Cc Thomas Gleixner <[email protected]>Signed-off-by Cyrill Gorcunov <[email protected]>--- fs/proc/base.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)Index:
linux-ml.git/fs/proc/base.c
===================================================================fs/proc/base.c +1
-1@@ -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" ,
fs/proc/base.c static int show_timer(struct seq_file *m · line 2299
Quoted 4 lines
fs/proc/base.c +1
-0@@ -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
Quoted 11 lines · Thomas Gleixner
fs/proc/base.c static int show_timer(struct seq_file *m · line 2299
Quoted 1 line
fs/proc/base.c +1
-0+ 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 1/1 fs/proc: timers -- Test for potential index overflow
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.
--- fs/proc/base.c | 6 ++++++ 1 file changed, 6 insertions(+)
fs/proc/base.c +6 -0
@@ -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
Quoted 5 lines · 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_notifyhas been fixed by Thomas in commitcef31d9af9082434, better to make sure we won't hit it again on furher modifications.
Quoted 8 lines
v2: Use explicit index overflow test (by tglx@). Since
it should never happen add warn-once to notify.Cc Andrey Vagin <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Ingo Molnar <[email protected]>Signed-off-by Cyrill Gorcunov <[email protected]>
Seems the patch is lost due to other duties. Guys, should I resend it or there some objections?