linux-kernel - Re: [PATCH v7 1/4] ipc: IPCMNI limit check for msgmni and shmmni

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] [day] [month] [year] [list]

Message-ID: <11228787-d9df-5c6e-3495-ad5ba2a16d18@redhat.com>
Date:   Mon, 18 Jun 2018 17:52:27 +0800
From:   Waiman Long <longman@...hat.com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org,
        Al Viro <viro@...iv.linux.org.uk>,
        Matthew Wilcox <willy@...radead.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH v7 1/4] ipc: IPCMNI limit check for msgmni and shmmni

On 05/10/2018 03:32 AM, Luis R. Rodriguez wrote:
> On Mon, May 07, 2018 at 07:57:12PM -0400, Waiman Long wrote:
>> On 05/07/2018 06:39 PM, Luis R. Rodriguez wrote:
>>> On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote:
>>>> A user can write arbitrary integer values to msgmni and shmmni sysctl
>>>> parameters without getting error, but the actual limit is really
>>>> IPCMNI (32k). This can mislead users as they think they can get a
>>>> value that is not real.
>>>>
>>>> The right limits are now set for msgmni and shmmni so that the users
>>>> will become aware if they set a value outside of the acceptable range.
>>>>
>>>> Signed-off-by: Waiman Long <longman@...hat.com>
>>>> ---
>>>>  ipc/ipc_sysctl.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
>>>> index 8ad93c2..f87cb29 100644
>>>> --- a/ipc/ipc_sysctl.c
>>>> +++ b/ipc/ipc_sysctl.c
>>>> @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>>>  static int zero;
>>>>  static int one = 1;
>>>>  static int int_max = INT_MAX;
>>>> +static int ipc_mni = IPCMNI;
>>>>  
>>>>  static struct ctl_table ipc_kern_table[] = {
>>>>  	{
>>>> @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>>>  		.data		= &init_ipc_ns.shm_ctlmni,
>>>>  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
>>>>  		.mode		= 0644,
>>>> -		.proc_handler	= proc_ipc_dointvec,
>>>> +		.proc_handler	= proc_ipc_dointvec_minmax,
>>>> +		.extra1		= &zero,
>>>> +		.extra2		= &ipc_mni,
>>>>  	},
>>>>  	{
>>>>  		.procname	= "shm_rmid_forced",
>>>> @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>>>  		.mode		= 0644,
>>>>  		.proc_handler	= proc_ipc_dointvec_minmax,
>>>>  		.extra1		= &zero,
>>>> -		.extra2		= &int_max,
>>>> +		.extra2		= &ipc_mni,
>>>>  	},
>>>>  	{
>>>>  		.procname	= "auto_msgmni",
>>>> -- 
>>>> 1.8.3.1
>>> It seems negative values are not allowed, if true then having
>>> a caller to use proc_douintvec_Fminmax() would help with ensuring
>>> no invalid negative input values are used as well.
>>>
>>>   Luis
>> Negative value doesn't mean sense here. So it is true that we can use
>> proc_douintvec_minmax() instead. However, the data types themselves are
>> defined as "int". So I think it is better to keep using
>> proc_dointvec_minmax() to be consistent with the data type.
> Huh, no... If you *know* the valid values *are* only positive, the right
> thing to do is to then *change* the data type. Tons of odd bugs can creep
> up because of these stupid things.
>
>   Luis

Sorry for the late reply.

First of all, negative value will not be accepted because of the zero
lower limit check. The type of msgmni, shmmni and semmni are defined as
int in the uapi/linux/msg.h and uapi/linux/shm.h and uapi/linux/sem.h.
They are exposed to the userspace and changing them to "unsigned int"
may cause some undesirable consequence. Again this is a case of
introducing risk without any noticeable benefit.

I understand your desire of cleaning thing up. However, I am hesitant to
take this risk without seeing any real benefit in this case.

Cheers,
Longman