linux-kernel - Re: [PATCH] ipc: Limit sysctl value to IPCMNI

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: <b4162bb3-3cb2-bd86-63f5-da4f92079745@redhat.com>
Date:   Mon, 18 Jun 2018 13:27:24 +0800
From:   Waiman Long <longman@...hat.com>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        Takashi Iwai <tiwai@...e.de>
Cc:     Davidlohr Bueso <dbueso@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ipc: Limit sysctl value to IPCMNI

On 06/13/2018 07:36 AM, Andrew Morton wrote:
> On Tue, 12 Jun 2018 08:36:32 +0200 Takashi Iwai <tiwai@...e.de> wrote:
>
>>> Well the present behaviour is to convert higher values downwards, yes?
>>>
>>> int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
>>> {
>>> 	kuid_t euid;
>>> 	kgid_t egid;
>>> 	int id, err;
>>>
>>> 	if (limit > IPCMNI)
>>> 		limit = IPCMNI;
>>>
>>> So if someone out there is presently setting this to 999999 then their
>>> kernel will work just fine.  After your proposed change, it will no
>>> longer do so - the tuning attempt will fail with -EINVAL.
>>>
>>> It really does us no good to say "you shouldn't have been doing that". 
>>> The fact that they *are* doing it and that it works OK is the kernel
>>> developers' fault for not applying suitable checking on day one.  I
>>> think we're stuck with continuing to accept such input.
>> Hm, that's one concern, yes.
>>
>> OTOH, we do secretly ignore the input value, and this isn't what's
>> expected by user, either.  Moreover, user-space has no slightest idea
>> which value can be accepted and which not.
>>
>> Actually I posted it just because of requests from customers who
>> needed to raise the bar, but didn't notice the effect.

My patchset actually contains patch to increase the IPC limit to over 32k.

>>
>> Maybe another possible solution would be to add another proc entry to
>> handle this correctly, and make the old one only for compatibility.
> Yes, I guess so.  Just leave the old tunable alone.  Possibly we
> could add a printk_once("please switch to the new interface").

A problem with that is the fact that those sysctl parameters have been
there for so long that it may be hard to persuade users to use the new
sysctl parameters. Earlier version of the patchset had added a clamping
minmax function to address that issue. However, it was deemed overkill
for adding quite a lot of code with not that much benefit.

Another alternative is to add a new sysctl parameter to turn on compat
mode where no error will be reported if it exceeds the limit just like
before. So a sysadmin can turn on compat mode if reporting range error
is an issue for existing applications.

Cheers,
Longman