From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Brauner Subject: Re: [PATCH net-next 2/2 v3] netns: restrict uevents Date: Sat, 28 Apr 2018 21:13:05 +0200 Message-ID: <20180428191304.GB13787@gmail.com> References: <20180427102306.8617-1-christian.brauner@ubuntu.com> <20180427102306.8617-3-christian.brauner@ubuntu.com> <87po2k7gt9.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, avagin@virtuozzo.com, ktkhai@virtuozzo.com, serge@hallyn.com, gregkh@linuxfoundation.org To: "Eric W. Biederman" Return-path: Content-Disposition: inline In-Reply-To: <87po2k7gt9.fsf@xmission.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Fri, Apr 27, 2018 at 11:30:26AM -0500, Eric W. Biederman wrote: > Christian Brauner writes: > > --- > > lib/kobject_uevent.c | 140 ++++++++++++++++++++++++++++++------------- > > 1 file changed, 99 insertions(+), 41 deletions(-) > > > > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c > > index c3cb110f663b..d8ce5e6d83af 100644 > > --- a/lib/kobject_uevent.c > > +++ b/lib/kobject_uevent.c > > > > +static int uevent_net_broadcast_tagged(struct sock *usk, > > + struct kobj_uevent_env *env, > > + const char *action_string, > > + const char *devpath) > > +{ > > + struct user_namespace *owning_user_ns = sock_net(usk)->user_ns; > > + struct sk_buff *skb = NULL; > > + int ret; > > + > > + skb = alloc_uevent_skb(env, action_string, devpath); > > + if (!skb) > > + return -ENOMEM; > > + > > + /* fix credentials */ > > + if (owning_user_ns != &init_user_ns) { > > Nit: This test is just a performance optimization as such is not > necessary. That is we can safely unconditionally set the > credentials this way. alloc_uevent_skb() will now set parms = &NETLINK_CB(skb); parms->creds.uid = GLOBAL_ROOT_UID; parms->creds.gid = GLOBAL_ROOT_GID; parms->dst_group = 1; parms->portid = 0; explicitly. So repeating that initialization unconditionally here does not make sense to me. Also, this hits map_uid_down() in user_namespace.c which is a known-hotpath (Remember the extensive testing we did back for uidmap limit bumping from 5 to 340.). And even though it might not matter much in this case there's no need to hit this code. The condition also make it obvious that only non-initial user namespace uevent sockets need fixing. Christian > > > + struct netlink_skb_parms *parms = &NETLINK_CB(skb); > > + kuid_t root_uid; > > + kgid_t root_gid; > > + > > + /* fix uid */ > > + root_uid = make_kuid(owning_user_ns, 0); > > + if (!uid_valid(root_uid)) > > + root_uid = GLOBAL_ROOT_UID; > > + parms->creds.uid = root_uid; > > + > > + /* fix gid */ > > + root_gid = make_kgid(owning_user_ns, 0); > > + if (!gid_valid(root_gid)) > > + root_gid = GLOBAL_ROOT_GID; > > + parms->creds.gid = root_gid; > > + } > > + > > + ret = netlink_broadcast(usk, skb, 0, 1, GFP_KERNEL); > > + /* ENOBUFS should be handled in userspace */ > > + if (ret == -ENOBUFS || ret == -ESRCH) > > + ret = 0; > > + > > + return ret; > > +} > > +#endif