From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH] userns: Replace netlink uses of cap_raised with capable. Date: Tue, 03 Apr 2012 19:45:47 -0700 Message-ID: References: <20120404022652.GA18730@mail.hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: In-Reply-To: <20120404022652.GA18730@mail.hallyn.com> (Serge E. Hallyn's message of "Wed, 4 Apr 2012 02:26:52 +0000") Sender: linux-kernel-owner@vger.kernel.org To: "Serge E. Hallyn" Cc: Andrew Morton , Philipp Reisner , Patrick McHardy , Andrew Morgan , Vasiliy Kulikov , David Howells , Neil Brown , Michal Januszewski , linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-raid@vger.kernel.org List-Id: linux-raid.ids "Serge E. Hallyn" writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> >> In 2009 Philip Reiser notied that a few users of netlink connector >> interface needed a capability check and added the idiom >> cap_raised(nsp->eff_cap, CAP_SYS_ADMIN) to a few of them, on the premise >> that netlink was asynchronous. >> >> In 2011 Patrick McHardy noticed we were being silly because netlink is >> synchronous and removed eff_cap from the netlink_skb_params and changed >> the idiom to cap_raised(current_cap(), CAP_SYS_ADMIN). >> >> Looking at those spots with a fresh eye we should be calling >> capable(CAP_SYS_ADMIN). The only reason I can see for not calling >> capable is that it once appeared we were not in the same task as the >> caller which would have made calling capable() impossible. > > And (just to make sure) that is now absolutely not the case? Absolutely. Netlink is synchronous. I reread netlink_unicast_kernel() in net/netlink/af_netlink.c while researching this patch. Netlink has been synchronous since 2007. In a world of perfect knowledge transmission these calls could have been calls to capable from the very beginning. >> In the initial user_namespace the only difference between between >> cap_raised(current_cap(), CAP_SYS_ADMIN) and capable(CAP_SYS_ADMIN) >> are a few sanity checks and the fact that capable(CAP_SYS_ADMIN) >> sets PF_SUPERPRIV if we use the capability. >> >> Since we are going to be using root privilege setting PF_SUPERPRIV >> seems the right thing to do. >> >> The motivation for this that patch is that in a child user namespace >> cap_raised(current_cap(),...) tests your capabilities with respect to >> that child user namespace not capabilities in the initial user namespace >> and thus will allow processes that should be unprivielged to use the >> kernel services that are only protected with >> cap_raised(current_cap(),..). >> >> To fix possible user_namespace issues and to just clean up the code >> replace cap_raised(current_cap(), CAP_SYS_ADMIN) with >> capable(CAP_SYS_ADMIN). >> >> Cc: Patrick McHardy >> Cc: Philipp Reisner >> Cc: Serge E. Hallyn > > Acked-by: Serge E. Hallyn > > thanks, > -serge > >> Cc: Andrew Morgan >> Cc: Vasiliy Kulikov >> Cc: David Howells >> Signed-off-by: Eric W. Biederman >> --- >> drivers/block/drbd/drbd_nl.c | 2 +- >> drivers/md/dm-log-userspace-transfer.c | 2 +- >> drivers/video/uvesafb.c | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c >> index abfaaca..946166e 100644 >> --- a/drivers/block/drbd/drbd_nl.c >> +++ b/drivers/block/drbd/drbd_nl.c >> @@ -2297,7 +2297,7 @@ static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms >> return; >> } >> >> - if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) { >> + if (!capable(CAP_SYS_ADMIN)) { >> retcode = ERR_PERM; >> goto fail; >> } >> diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c >> index 1f23e04..08d9a20 100644 >> --- a/drivers/md/dm-log-userspace-transfer.c >> +++ b/drivers/md/dm-log-userspace-transfer.c >> @@ -134,7 +134,7 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp) >> { >> struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1); >> >> - if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) >> + if (!capable(CAP_SYS_ADMIN)) >> return; >> >> spin_lock(&receiving_list_lock); >> diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c >> index 260cca7..9f7d27a 100644 >> --- a/drivers/video/uvesafb.c >> +++ b/drivers/video/uvesafb.c >> @@ -73,7 +73,7 @@ static void uvesafb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *ns >> struct uvesafb_task *utask; >> struct uvesafb_ktask *task; >> >> - if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) >> + if (!capable(CAP_SYS_ADMIN)) >> return; >> >> if (msg->seq >= UVESAFB_TASKS_MAX) >> -- >> 1.7.2.5