linux-kernel - Re: [PATCH] ipc,shm: disable shmmax and shmall by default

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: <5348FE43.1070508@colorfullife.com>
Date:	Sat, 12 Apr 2014 10:50:11 +0200
From:	Manfred Spraul <manfred@...orfullife.com>
To:	Davidlohr Bueso <davidlohr@...com>
CC:	KOSAKI Motohiro <kosaki.motohiro@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, aswin@...com,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	Greg Thelen <gthelen@...gle.com>,
	Kamezawa Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH] ipc,shm: disable shmmax and shmall by default

On 04/11/2014 10:27 PM, Davidlohr Bueso wrote:
> On Fri, 2014-04-11 at 20:28 +0200, Manfred Spraul wrote:
>> Hi Davidlohr,
>>
>> On 04/03/2014 02:20 AM, Davidlohr Bueso wrote:
>>> The default size for shmmax is, and always has been, 32Mb.
>>> Today, in the XXI century, it seems that this value is rather small,
>>> making users have to increase it via sysctl, which can cause
>>> unnecessary work and userspace application workarounds[1].
>>>
>>> [snip]
>>> Running this patch through LTP, everything passes, except the following,
>>> which, due to the nature of this change, is quite expected:
>>>
>>> shmget02    1  TFAIL  :  call succeeded unexpectedly
>> Why is this TFAIL expected?
> So looking at shmget02.c, this is the case that fails:
>
> 		for (i = 0; i < TST_TOTAL; i++) {
> 			/*
> 			 * Look for a failure ...
> 			 */
>
> 			TEST(shmget(*(TC[i].skey), TC[i].size, TC[i].flags));
>
> 			if (TEST_RETURN != -1) {
> 				tst_resm(TFAIL, "call succeeded unexpectedly");
> 				continue;
> 			}
>
> Where TC[0] is:
> struct test_case_t {
> 	int *skey;
> 	int size;
> 	int flags;
> 	int error;
> } TC[] = {
> 	/* EINVAL - size is 0 */
> 	{
> 	&shmkey2, 0, IPC_CREAT | IPC_EXCL | SHM_RW, EINVAL},
>
> So it's expected because now 0 is actually valid. And before:
>
>   EINVAL A new segment was to be created and size < SHMMIN or size > SHMMAX
>
>>> diff --git a/ipc/shm.c b/ipc/shm.c
>>> index 7645961..ae01ffa 100644
>>> --- a/ipc/shm.c
>>> +++ b/ipc/shm.c
>>> @@ -490,10 +490,12 @@ static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>>>    	int id;
>>>    	vm_flags_t acctflag = 0;
>>>    
>>> -	if (size < SHMMIN || size > ns->shm_ctlmax)
>>> +	if (ns->shm_ctlmax &&
>>> +	    (size < SHMMIN || size > ns->shm_ctlmax))
>>>    		return -EINVAL;
>>>    
>>> -	if (ns->shm_tot + numpages > ns->shm_ctlall)
>>> +	if (ns->shm_ctlall &&
>>> +	    ns->shm_tot + numpages > ns->shm_ctlall)
>>>    		return -ENOSPC;
>>>    
>>>    	shp = ipc_rcu_alloc(sizeof(*shp));
>> Ok, I understand it:
>> Your patch disables checking shmmax, shmall *AND* checking for SHMMIN.
> Right, if shmmax is 0, then there's no point checking for shmmin,
> otherwise we'd always end up returning EINVAL.
>
>> a) Have you double checked that 0-sized shm segments work properly?
>>    Does the swap code handle it properly, ...? EINVAL A new segment was to be created and size < SHMMIN or size > SHMMAX
> Hmm so I've been using this patch just fine on my laptop since I sent
> it. So far I haven't seen any issues. Are you refering to something in
> particular? I'd be happy to run any cases you're concerned with.
I'm thinking about malicious applications.
Create 0-sized segments and then map them. Does find_vma_intersection 
handle that case?
The same for all other functions that are called by the shm code.

You can't replace code review by "runs for a month"
>> b) It's that yet another risk for user space incompatibility?
> Sorry, I don't follow here.
Applications expect that shmget(,0,) fails.

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