linux-kernel - Re: [PATCH 1/2] kernel/sys.c: return the current gid when error occurs

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: <5201BF49.4060305@asianux.com>
Date:	Wed, 07 Aug 2013 11:30:17 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Andy Lutomirski <luto@...capital.net>
CC:	Kees Cook <keescook@...omium.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Oleg Nesterov <oleg@...hat.com>, holt@....com,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] kernel/sys.c: return the current gid when error occurs

On 08/07/2013 04:21 AM, Andy Lutomirski wrote:
> On 08/06/2013 01:01 AM, Chen Gang wrote:
>> According to the API definition, when error occurs, need return current
>> fsgid instead of the previous one.
>>
>> The related informations ("man setfsgid"):
>>
>>   RETURN VALUE
>>          On success, the previous value of fsgid is returned.  On error, the current value of fsgid is returned.
>>
>> Signed-off-by: Chen Gang <gang.chen@...anux.com>
>> ---
>>  kernel/sys.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 771129b..9356dc8 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -775,11 +775,11 @@ SYSCALL_DEFINE1(setfsgid, gid_t, gid)
>>  
>>  	kgid = make_kgid(old->user_ns, gid);
>>  	if (!gid_valid(kgid))
>> -		return old_fsgid;
>> +		return gid;
> 
> Huh?  So if 1234567 is not a valid gid, then setfsgid(1234567) is
> supposed to return 1234567?  This makes no sense.
>

Hmm... one explanation is "current value" means the value which will be
set, it may be incorrect, if so, need return with failure (at least, it
is not conflict with the meaning of "current value").

For using:

  Assume the caller's new fsgid is different with the previous one (or the caller need/should not call it).
  If the return value is not equal to the new fsgid, that means the system call succeeds (it is the previous one which may be useful for caller, too).
  If the return value is equal to the new fsgid (the new fsgid may '1234567'), that means the system call fails.

So at least, "man page" description is correct.

> I assume that what the man page means is that the return value is
> whatever fsgid was prior to the call.  On error, fsgid isn't changed, so
> the return value is still "current".
> 

Hmm... which 'variable' can be qualified as "current value" ?

I think, at least it should match 3 requirements.

  1. it must be different with "the previous value" (or "current value" is meaningless).
  2. it must be known about by the caller (or caller can not check whether fails or not).
  3. its meaning should not be conflict with "current value" (if "current value" is incorrect, it should be fail).

It seems only the parameter gid which caller inputs is qualified with
the 3 requirements.

> (FWIW, this behavior is awful and is probably the cause of a security
> bug or three, since success and failure are indistinguishable.  But I
> think your patch is wrong.)
> 

Hmm... at least, the original implementation is incorrect, and we need
discussing for this issues.

> --Andy
> 
> 

Thanks.
-- 
Chen Gang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/