linux-kernel - Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Jul 2019 10:23:38 +0300
From:   Cyrill Gorcunov <gorcunov@...il.com>
To:     Yang Xu <xuyang2018.jy@...fujitsu.com>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling
 PR_SET_TIMERSLACK

On Tue, Jul 23, 2019 at 11:30:53AM +0800, Yang Xu wrote:
> arg2 will never < 0, for its type is 'unsigned long'. So negative
> judgment is meaningless.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@...fujitsu.com>
> ---
>  kernel/sys.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 2969304c29fe..399457d26bef 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  			error = current->timer_slack_ns;
>  		break;
>  	case PR_SET_TIMERSLACK:
> -		if (arg2 <= 0)
> +		if (arg2)
> +			current->timer_slack_ns = arg2;
> +		else
>  			current->timer_slack_ns =
>  					current->default_timer_slack_ns;
> -		else
> -			current->timer_slack_ns = arg2;
>  		break;
>  	case PR_MCE_KILL:
>  		if (arg4 | arg5)

>From a glance it looks correct to me, but then...

1) you might simply compare with zero, iow if (arg2 == 0)
   instead of changing 7 lines
2) according to man page passing negative value should be acceptable,
   though it never worked as expected. I've been grepping "git log"
   for this file and the former API is coming from

commit 6976675d94042fbd446231d1bd8b7de71a980ada
Author: Arjan van de Ven <arjan@...ux.intel.com>
Date:   Mon Sep 1 15:52:40 2008 -0700

    hrtimer: create a "timer_slack" field in the task struct

which is 11 years old by now. Nobody complained so far even when man
page is saying pretty obviously

       PR_SET_TIMERSLACK (since Linux 2.6.28)
              Each thread has two associated timer slack values:  a  "default"
              value, and a "current" value.  This operation sets the "current"
              timer slack value for the calling  thread.   If  the  nanosecond
              value  supplied in arg2 is greater than zero, then the "current"
              value is set to this value.  If arg2 is less than  or  equal  to
              zero,  the  "current"  timer  slack  is  reset  to  the thread's
              "default" timer slack value.

So i think to match the man page (and assuming that accepting negative value
has been supposed) we should rather do

	if ((long)arg2 < 0)

Thoughts?