linux-kernel - Re: [PATCH] kernel/groups.c: consider about NULL for 'group

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: <524114B8.3070903@asianux.com>
Date:	Tue, 24 Sep 2013 12:27:36 +0800
From:	Chen Gang <gang.chen@...anux.com>
To:	Tejun Heo <tj@...nel.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [PATCH] kernel/groups.c: consider about NULL for 'group_info'
 in all related extern functions

On 09/24/2013 12:06 PM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Sep 24, 2013 at 11:42:56AM +0800, Chen Gang wrote:
>> Hmm... can user be permitted to call other system call (e.g. getgroups)
>> before call groups_alloc()? (may the user space already give check, but
>> for our kernel, we can not only depend on their checking).
> 
> I don't think so.
> 
>> According to group_alloc() and setgroups() usage in kernel source code,
>> 'group_info' may be not set if kernel/process is running (although user
>> space may be sure "if kernel is running, 'group_info' must be set").
>>
>> The below is the proof for "kernel itself can not be sure 'group_info'
>> must be set during kernel/process is running", please check, thanks.
> ..
>> The related conclusion:
>>
>>   during kernel startup or process creation, kernel does not intend to set 'group_info'.
> 
> No, this is not a proof or any meaningful conclusion.  This is just
> some random suspicions combined with supposedly related grep output.
> 

For real world, /sbin/init will call setgroups, so user space 'help'
kernel itself to protect this issue, but I think, we don't only depend
on the user space help checking.

The proof is below:

  [root@...enlinux tmp]# grep setgroups /sbin/*
  Binary file /sbin/init matches
  Binary file /sbin/rpc.statd matches
  Binary file /sbin/rsyslogd matches
  Binary file /sbin/runuser matches

>From reading kernel source code, kernel itself does not intend to set
'group_info', it is triggered by user space or another kernel mode
sub-systems.


>> In extern function groups_search (which also called by export function
>> in_group_p and in_egroup_p), it checks "if 'cred->group_info' is NULL".
>>
>> So "kernel/groups.c" have 9 extern/export/system-call functions, and
>> 4/9 notice about "if 'cred->group_info' is NULL" (e.g. groups_alloc,
>> groups_search, in_group_p, in_egroup_p).
>>
>> So for API self-consistency, all of extern/export/system-call functions
>> need notice about it.
> 
> I'm afraid this isn't useful.  If you want to change the code, you
> actually need to understand what's going on.  "this seems weird to me"
> is a good starting point but you need to go way beyond that to
> actually make changes.
> 

If some of APIs check the related parameters, but the other not, this
interface must assume some usage condition from the callers which
callers can break out (although the callers may not break out, now).

> 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/