Messages in this thread |  | | From | Christian Brauner <> | Date | Sun, 16 Jun 2019 18:50:28 +0200 | Subject | Re: [PATCH net-next 2/2 v5] netns: restrict uevents |
| |
On Sun, Jun 16, 2019 at 06:50:20AM -0500, Eric W. Biederman wrote: > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: > > > Hi Christian, > > > > On Sun, Apr 29, 2018 at 3:45 AM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > >> > >> commit 07e98962fa77 ("kobject: Send hotplug events in all network namespaces") > >>abhishekbh@google.com > >> enabled sending hotplug events into all network namespaces back in 2010. > >> Over time the set of uevents that get sent into all network namespaces has > >> shrunk. We have now reached the point where hotplug events for all devices > >> that carry a namespace tag are filtered according to that namespace. > >> Specifically, they are filtered whenever the namespace tag of the kobject > >> does not match the namespace tag of the netlink socket. > >> Currently, only network devices carry namespace tags (i.e. network > >> namespace tags). Hence, uevents for network devices only show up in the > >> network namespace such devices are created in or moved to. > >> > >> However, any uevent for a kobject that does not have a namespace tag > >> associated with it will not be filtered and we will broadcast it into all > >> network namespaces. This behavior stopped making sense when user namespaces > >> were introduced. > >> > >> This patch simplifies and fixes couple of things: > >> - Split codepath for sending uevents by kobject namespace tags: > >> 1. Untagged kobjects - uevent_net_broadcast_untagged(): > >> Untagged kobjects will be broadcast into all uevent sockets recorded > >> in uevent_sock_list, i.e. into all network namespacs owned by the > >> intial user namespace. > >> 2. Tagged kobjects - uevent_net_broadcast_tagged(): > >> Tagged kobjects will only be broadcast into the network namespace they > >> were tagged with. > >> Handling of tagged kobjects in 2. does not cause any semantic changes. > >> This is just splitting out the filtering logic that was handled by > >> kobj_bcast_filter() before. > >> Handling of untagged kobjects in 1. will cause a semantic change. The > >> reasons why this is needed and ok have been discussed in [1]. Here is a > >> short summary: > >> - Userspace ignores uevents from network namespaces that are not owned by > >> the intial user namespace: > >> Uevents are filtered by userspace in a user namespace because the > >> received uid != 0. Instead the uid associated with the event will be > >> 65534 == "nobody" because the global root uid is not mapped. > >> This means we can safely and without introducing regressions modify the > >> kernel to not send uevents into all network namespaces whose owning > >> user namespace is not the initial user namespace because we know that > >> userspace will ignore the message because of the uid anyway. > >> I have a) verified that is is true for every udev implementation out > >> there b) that this behavior has been present in all udev > >> implementations from the very beginning. > > > > Unfortunately udev is not the only consumer of uevents, for example on > > Android there is healthd that also consumes uevents, and this > > particular change broke Android running in a container on Chrome OS. > > Can this be reverted? Or, if we want to keep this, how can containers > > that use separate user namespace still listen to uevents? > > The code has been in the main tree for over a year so at a minimum > reverting this has the real chance of causing a regression for > folks like lxc. > > I don't think Android running in a container on Chrome OS was even > available when this change was merged. So I don't think this falls > under the ordinary no regression rules. > > I may be wrong but I think this is a case of developing new code on an > old kernel and developing a dependence on a bug that had already been > fixed in newer kernels. I know Christian did his best to reach out to > everyone when this change came through, so only getting a bug report > over a year after the code was merged is concerning. > > That said uevents should be completely useless in a user namespace > except as letting you know something happened. Is that what healthd > is using them for? > > > One solution would be to tweak the container userspace on ChromeOS to > listen to the uevents outside the container and to relay them into the > Android container.
Thanks for jumping in Eric! Welcome back. :)
Hey Dmitry,
Crostini on ChromeOS is making heavy use of this patchset and of LXD. So reverting this almost 1.5 years after the fact will regress all of Google's ChromeOS Crostini users, and all LXD/LXC users.
LXD and Crostini by using LXD (through Concierge/Tremplin etc. [2]) are using this whole series e.g. when hotplugging usb devices into the container.
When a usb hotplug happens, LXD will receive the relevant uevent and will forward it to the container. Any process listening on a uevent socket inside the container will now be able to see it.
Now, to talk briefly about solutions: From what I gather from talking to the ChromeOS guys and from your ChromeOS bugtracker and recent patchsets to ARC you are moving your Android workloads into Crostini? So like Eric said this seems like a new feature you're implementing.
If you need to be able to listen to uevents inside of a user namespace and plan on using Crostini going forward then you can have Crostini forward battery uevents to the container. The logic for doing this can be found in the LXD codebase (cf. [3]). It's pretty simple. If you want to go this route I'm happy to guide you. :) Note, both options are a version of what Eric suggested in his last paragraph!
What astonishes me is that healthd couldn't have possibly received battery uevents for a long time even if Android already was run in user namespaces prior to the new feature you're working on and the healthd I see in master is not even using uevents anymore (cf. [8]) but rather is using sysfs it seems. :) Before that healthd was using (cf. [7])
uevent_kernel_multicast_recv() | -> uevent_kernel_multicast_uid_recv
the latter containing the check
if (cred->uid != 0) { /* ignoring netlink message from non-root user */ goto out; }
Before my patchset here the uevents sent out came with cred->uid == INVALID_UID and so healthd never received those events until very recently. And I can tell you exactly when it started receiving those events as I reported the removal of this check as a bug against Android before this patchset was ever merged and before a version of Android without this check was released (cf. [6]). :) While we're at it, removing this check is strange. Why would you have any core tool of yours listen to uevents that do not come from the root user? Especially when it comes from INVALID_UID. That's what I tried to tell you in [6].
This patchset also fixes a real information leak. Netlink is a socket and having those sockets respect network namespaces like all others makes sense. Especially to allow containers to do their own thing and to avoid information leakage.
Fyi, when I wrote this patch I informed the ChromeOS guys about it multiple times as early as March/April 2018. At least two times the PM in person (last time we talked about this was during FOSDEM because ChromeOS was already reling on this). I also mentioned this feature on the official ChromeOS bugtracker here [1] in April 2018 since ChromeOS could use it. So that this comes up as a problem almost 1.5 years later is a bit surprising. :) Additionally, I gave a presentation about this feature at LPC 2018 with Google folks from Android around as well (cf. [4]). :)
Thanks! Christian
[1]: https://bugs.chromium.org/p/chromium/issues/detail?id=831850#c2 [2]: https://bugs.chromium.org/p/chromium/issues/detail?id=956288 [3]: https://github.com/lxc/lxd [4]: https://www.youtube.com/watch?v=yI_1cuhoDgA&t=327s [5]: https://android.googlesource.com/platform/system/core.git/+/android-4.2.2_r1/libcutils/uevent.c#77 [6]: https://issuetracker.google.com/issues/77764945 [7]: https://android.googlesource.com/platform/system/core/+/android-4.4_r1/healthd/healthd.cpp#135 [8]: https://android.googlesource.com/platform/system/core/+/master/healthd/BatteryMonitor.cpp
|  |