[PATCH] userns: Replace netlink uses of cap_raised with capable.

[PATCH] userns: Replace netlink uses of cap_raised with capable.

[Eric W. Biederman]

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.

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>
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

Re: [PATCH] userns: Replace netlink uses of cap_raised with capable.

[Serge E. Hallyn]

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

Re: [PATCH] userns: Replace netlink uses of cap_raised with capable.

[Eric W. Biederman]

"Serge E. Hallyn" <serge@hallyn.com> 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 <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

Re: [PATCH] userns: Replace netlink uses of cap_raised with capable.

[James Morris]

On Tue, 3 Apr 2012, Eric W. Biederman wrote:

> 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.

Agreed.

Reviewed-by: James Morris <james.l.morris@oracle.com>


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] userns: Replace netlink uses of cap_raised with capable.

[Andrew G. Morgan]

Acked-by: Andrew G. Morgan <morgan@kernel.org>

Cheers

Andrew

On Tue, Apr 3, 2012 at 7:45 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Serge E. Hallyn" <serge@hallyn.com> 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 <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