netdev - Re: [RFC][PATCH] IP address restricting cgroup subsystem

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: <496476FD.8090209@cn.fujitsu.com>
Date:	Wed, 07 Jan 2009 17:33:49 +0800
From:	Li Zefan <lizf@...fujitsu.com>
To:	Grzegorz Nosek <root@...aldomain.pl>
CC:	containers@...ts.osdl.org, netdev@...r.kernel.org
Subject: Re: [RFC][PATCH] IP address restricting cgroup subsystem

Grzegorz Nosek wrote:
> On śro, sty 07, 2009 at 04:36:35 +0800, Li Zefan wrote:
>> Grzegorz Nosek wrote:
>>>>> IP addresses are write-once (via /cgroup/.../ipaddr.ipv4 in dotted-quad
>>>> Why they should be write-once ?
>>> No real (technical) reason. Making it read-write would be fine with me.
>>> I wanted to make the restriction a one-way road but I guess I can police
>>> that in userspace (simply don't write anything to the file twice).
>>>
>> But seems the patch makes it impossible to re-allow a restricted task to
>> be binded to INADDR_ANY.
> 
> Yes, my goal is to disallow that but I don't insist to do that in the
> kernel (I'm not currently planning to let untrusted root loose in a
> container).
> 
>> Firstly, is inheritance necessary ?
> 
> It would be nice to have when the container's root is untrusted but
> might want to subdivide the container's cgroup for other purposes.
> Without inheritance, they would be able to circumvent the IP address
> restriction. One could argue that a full untrusted-root container would
> need a proper network namespace anyway (and giving CAP_SYS_ADMIN there
> is probably a very bad idea), but still, I'd feel uneasy.
> 
>> If yes, then how about:
>>
>> The root cgroup is read-only, so the tasks in it always bind to INADDR_ANY.
>> For other cgroups, write is allowed only if it has no children and the
>> parent is INADDR_ANY.
> 
> Yes, I like that. Will update the patch. I assume that I must check
> list_empty(&cgroup->children)?

Yes.

> Should I use cgroup_lock()/cgroup_unlock()

Yes.

> or other locking? I think it will be safe to do without locks but would
> rather get some expert advice.
> 

No. Without locks, it races with mkdir.

=============

//cgroup_lock();

if (list_empty(&cgrp->children) &&
    parent->ipv4_addr == INADDR_ANY)
					   <--- mkdir()
	ipcgroup->ipv4_addr = new_addr;

//cgroup_unlock();

==============

In the above case, ipcgroup->ipv4_addr = new_addr,
but child_cgroup->ipv4_addr == INADDR_ANY, which is not expected.

> Thanks a lot for your comments.
> 
> Best regards,
>  Grzegorz Nosek
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html