Messages in this thread | | | Date | Wed, 4 Apr 2012 02:26:52 +0000 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH] userns: Replace netlink uses of cap_raised with capable. |
| |
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?
> 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 <kaber@trash.net> > Cc: Philipp Reisner <philipp.reisner@linbit.com> > Cc: Serge E. Hallyn <serge.hallyn@canonical.com>
Acked-by: Serge E. Hallyn <serge.hallyn@canonical.com>
thanks, -serge
> Cc: Andrew Morgan <morgan@kernel.org> > Cc: Vasiliy Kulikov <segoon@openwall.com> > Cc: David Howells <dhowells@redhat.com> > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > 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
| |