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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5D36C11D.1070804@cn.fujitsu.com>
Date:   Tue, 23 Jul 2019 16:11:09 +0800
From:   Yang Xu <xuyang2018.jy@...fujitsu.com>
To:     Cyrill Gorcunov <gorcunov@...il.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 2019/07/23 15:23, Cyrill Gorcunov wrote:

> 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
Hi Cyril

Indeed.  simply compare with zero might be better.

> 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)
Looks correct. But if we set a ULONG_MAX(PR_GET_TIMERSLACK also limits ULONG_MAX)
value(about 4s) on 32bit machine, this code will think this value is a negative value and use default value.

I guess man page was written as "less than or equal to zero" because of this confusing code(arg2<=0, but arg2
is an unsinged long value).
I think we can change this man page and also add bounds value description.

Also, I found a patch about arg2 is an unsigned long value

commit 7fe5e04292e71af34ae171b88caa2a139e0b6125
Author: Chen Gang<gang.chen@...anux.com>
Date:   Thu Feb 21 16:43:06 2013 -0800

     sys_prctl(): arg2 is unsigned long which is never<  0

     arg2 will never<  0, for its type is 'unsigned long'

     Also, use the provided macros.

What do you think about it ?

> Thoughts?
>
>
>