On Fri, Dec 21, 2018 at 04:59:19PM +0100, Christian Brauner wrote: > On Fri, Dec 21, 2018 at 04:37:58PM +0100, Greg KH wrote: > > On Fri, Dec 21, 2018 at 03:12:42PM +0100, Christian Brauner wrote: > > > On Fri, Dec 21, 2018 at 02:55:09PM +0100, Greg KH wrote: > > > > On Fri, Dec 21, 2018 at 02:39:09PM +0100, Christian Brauner wrote: > > > > > This implements three sysctls that have very specific goals: > > > > > > > > Ick, why? > > > > > > > > What are these going to be used for? Who will "control" them? As you > > > > > > Only global root in the initial user namespace. See the reasons below. :) > > > > > > > are putting them in the "global" namespace, that feels like something > > > > that binderfs was trying to avoid in the first place. > > > > > > There are a couple of reason imho: > > > - Global root needs a way to restrict how many binder devices can be > > > allocated across all user + ipc namespace pairs. > > > One obvious reason is that otherwise userns root in a non-initial user > > > namespace can allocate a huge number of binder devices (pick a random > > > number say 10.000) and use up a lot of kernel memory. > > > > Root can do tons of other bad things too, why are you picking on > > That's global root not userns root though. :) These sysctls are about > global root gaining the ability to proactively restrict binder device > delegation. > > > binderfs here? :) > > > > > In addition they can pound on the binder.c code causing a lot of > > > contention for the remaining global lock in there. > > > > That's the problem of that container, don't let it do that. Or remove > > the global lock :) > > > > > We should let global root explicitly restrict non-initial namespaces > > > in this respect. Imho, that's just good security design. :) > > > > If you do not trust your container enough to have it properly allocate > > the correct binder resources, then perhaps you shouldn't be allowing it > > to allocate any resources at all? > > Containers just like VMs get delegated and you might not have control > over what is running in there. That's AWS in a nutshell. :) Restricting > it by saying "just don't do that" seems not something that is > appropriate given the workloads out there in the wild. > In general, I do *understand* the reasoning but I think the premise is > flawed if we can somewhat trivially make this safe. > > > > > > - The reason for having a number of reserved devices is when the initial > > > binderfs mount needs to bump the number of binder devices after the > > > initial allocation done during say boot (e.g. it could've removed > > > devices and wants to reallocate new ones but all binder minor numbers > > > have been given out or just needs additional devices). By reserving an > > > initial pool of binder devices this can be easily accounted for and > > > future proofs userspace. This is to say: global root in the initial > > > userns + ipcns gets dibs on however many devices it wants. :) > > > > binder devices do not "come and go" at runtime, you need to set them up > > initially and then all is fine. So there should never be a need for the > > "global" instance to need "more" binder devices once it is up and > > running. So I don't see what you are really trying to solve here. > > That's dismissing a whole range of use-cases where you might allocate > and deallocate devices on the fly which this is somewhat designed for. > But I guess ok for now. > > > > > You seem to be trying to protect the system from the container you just > > gave root to and trusted it with creating its own binder instances. > > If you do not trust it to create binder instances then do not allow it > > to create binder instances! :) > > Again, I get the reasoning but think that this dismisses major > real-world use-cases not just for binderfs but for all instances where > untrusted workloads are run which both containers and VMs aim to make > sure are possible. > Note, I mean untrusted not in the sense of necessarily being malicious > but just "can't guarantee that things don't blow up in your face". > > > > > > - The fact that we have a single shared pool of binder device minor > > > numbers for all namespaces imho makes it necessary for the global root > > > user in the initial ipc + user namespace to manage device allocation > > > and delegation. > > > > You are managing the allocation, you are giving who ever asks for one a > > device. If you run out of devices, oops, you run out of devices, that's > > it. Are you really ever going to run out of a major's number of binder > > devices? > > The point is more about someone intentionally trying to do that. > > > > > > The binderfs sysctl stuff is really small code-wise and adds a lot of > > > security without any performance impact on the code itself. So we > > > actually very strictly adhere to the requirement to not blindly > > > sacrifice performance for security. :) > > > > But you are adding a brand new user/kernel api by emulating one that is > > very old and not the best at all, to try to protect from something that > > seems like you can't really "protect" from in the first place. > > Of course we can protect from that. It's about init root never running > out of devices. We don't care about non-init-userns running out of > devices at all. > > > > > You now have a mis-match of sysctls, ioctls and file operations all > > working on the same logical thing. And all interacting in different and > > uncertian ways. Are you sure that's wise? > > The sysctl approach is present in other pseudo-filesystems apart from > devpts. For example, mqueue. > > > > > If the binderfs code as-is isn't "safe enough" to use without this, then > > we need to revisit it before someone starts to use it... > > *It is safe.* I just don't see a good argument against additional > hardening. *But I'm definitely not going to push this patch if it's > additional hardening features are used to push the unsound argument that > binderfs isn't safe.* :) Ok, so what you really want is just "limits" to prevent a container from doing something really crazy, right? So, how about a limit of 10 binder nodes per container? Make it a kernel build option so it can be changed by a vendor if they really find that is a problem. Would that solve the issue you are thinking might be here? thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
