diff mbox series

posix-timer: Properly check sigevent->sigev_notify

Message ID 1533331532-25426-2-git-send-email-tyhicks@canonical.com
State New
Headers show
Series posix-timer: Properly check sigevent->sigev_notify | expand

Commit Message

Tyler Hicks Aug. 3, 2018, 9:25 p.m. UTC 
From: Thomas Gleixner <tglx@linutronix.de>

timer_create() specifies via sigevent->sigev_notify the signal delivery for
the new timer. The valid modes are SIGEV_NONE, SIGEV_SIGNAL, SIGEV_THREAD
and (SIGEV_SIGNAL | SIGEV_THREAD_ID).

The sanity check in good_sigevent() is only checking the valid combination
for the SIGEV_THREAD_ID bit, i.e. SIGEV_SIGNAL, but if SIGEV_THREAD_ID is
not set it accepts any random value.

This has no real effects on the posix timer and signal delivery code, but
it affects show_timer() which handles the output of /proc/$PID/timers. That
function uses a string array to pretty print sigev_notify. The access to
that array has no bound checks, so random sigev_notify cause access beyond
the array bounds.

Add proper checks for the valid notify modes and remove the SIGEV_THREAD_ID
masking from various code pathes as SIGEV_NONE can never be set in
combination with SIGEV_THREAD_ID.

Reported-by: Eric Biggers <ebiggers3@gmail.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: stable@vger.kernel.org

CVE-2017-18344

(backported from commit cef31d9af908243421258f1df35a4a644604efbe)
[tyhicks: Do not worry about removing the SIGEV_THREAD_ID masking since it is
 irrelevant to the security fix]
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 kernel/posix-timers.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Comments

Kleber Sacilotto de Souza Aug. 14, 2018, 3:19 p.m. UTC | #1 
On 08/03/18 23:25, Tyler Hicks wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> timer_create() specifies via sigevent->sigev_notify the signal delivery for
> the new timer. The valid modes are SIGEV_NONE, SIGEV_SIGNAL, SIGEV_THREAD
> and (SIGEV_SIGNAL | SIGEV_THREAD_ID).
> 
> The sanity check in good_sigevent() is only checking the valid combination
> for the SIGEV_THREAD_ID bit, i.e. SIGEV_SIGNAL, but if SIGEV_THREAD_ID is
> not set it accepts any random value.
> 
> This has no real effects on the posix timer and signal delivery code, but
> it affects show_timer() which handles the output of /proc/$PID/timers. That
> function uses a string array to pretty print sigev_notify. The access to
> that array has no bound checks, so random sigev_notify cause access beyond
> the array bounds.
> 
> Add proper checks for the valid notify modes and remove the SIGEV_THREAD_ID
> masking from various code pathes as SIGEV_NONE can never be set in
> combination with SIGEV_THREAD_ID.
> 
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Reported-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: stable@vger.kernel.org
> 
> CVE-2017-18344
> 
> (backported from commit cef31d9af908243421258f1df35a4a644604efbe)
> [tyhicks: Do not worry about removing the SIGEV_THREAD_ID masking since it is
>  irrelevant to the security fix]
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  kernel/posix-timers.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
> index 77e6b83c0431..26b910462a0c 100644
> --- a/kernel/posix-timers.c
> +++ b/kernel/posix-timers.c
> @@ -498,17 +498,22 @@ static struct pid *good_sigevent(sigevent_t * event)
>  {
>  	struct task_struct *rtn = current->group_leader;
>  
> -	if ((event->sigev_notify & SIGEV_THREAD_ID ) &&
> -		(!(rtn = find_task_by_vpid(event->sigev_notify_thread_id)) ||
> -		 !same_thread_group(rtn, current) ||
> -		 (event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_SIGNAL))
> +	switch (event->sigev_notify) {
> +	case SIGEV_SIGNAL | SIGEV_THREAD_ID:
> +		rtn = find_task_by_vpid(event->sigev_notify_thread_id);
> +		if (!rtn || !same_thread_group(rtn, current))
> +			return NULL;
> +		/* FALLTHRU */
> +	case SIGEV_SIGNAL:
> +	case SIGEV_THREAD:
> +		if (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX)
> +			return NULL;
> +		/* FALLTHRU */
> +	case SIGEV_NONE:
> +		return task_pid(rtn);
> +	default:
>  		return NULL;
> -
> -	if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) &&
> -	    ((event->sigev_signo <= 0) || (event->sigev_signo > SIGRTMAX)))
> -		return NULL;
> -
> -	return task_pid(rtn);
> +	}
>  }
>  
>  void posix_timers_register_clock(const clockid_t clock_id,
>
diff mbox series

Patch

diff --git a/kernel/posix-timers.c b/kernel/posix-timers.c
index 77e6b83c0431..26b910462a0c 100644
--- a/kernel/posix-timers.c
+++ b/kernel/posix-timers.c
@@ -498,17 +498,22 @@  static struct pid *good_sigevent(sigevent_t * event)
 {
 	struct task_struct *rtn = current->group_leader;
 
-	if ((event->sigev_notify & SIGEV_THREAD_ID ) &&
-		(!(rtn = find_task_by_vpid(event->sigev_notify_thread_id)) ||
-		 !same_thread_group(rtn, current) ||
-		 (event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_SIGNAL))
+	switch (event->sigev_notify) {
+	case SIGEV_SIGNAL | SIGEV_THREAD_ID:
+		rtn = find_task_by_vpid(event->sigev_notify_thread_id);
+		if (!rtn || !same_thread_group(rtn, current))
+			return NULL;
+		/* FALLTHRU */
+	case SIGEV_SIGNAL:
+	case SIGEV_THREAD:
+		if (event->sigev_signo <= 0 || event->sigev_signo > SIGRTMAX)
+			return NULL;
+		/* FALLTHRU */
+	case SIGEV_NONE:
+		return task_pid(rtn);
+	default:
 		return NULL;
-
-	if (((event->sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) &&
-	    ((event->sigev_signo <= 0) || (event->sigev_signo > SIGRTMAX)))
-		return NULL;
-
-	return task_pid(rtn);
+	}
 }
 
 void posix_timers_register_clock(const clockid_t clock_id,