Patchwork [2/2] netlink: kill eff_cap from struct netlink_skb_parms

login
register
mail settings
Submitter Patrick McHardy
Date March 3, 2011, 9:38 a.m.
Message ID <4D6F6180.5030903@trash.net>
Download mbox | patch
Permalink /patch/85251/
State Deferred
Delegated to: David Miller
Headers show

Comments

Patrick McHardy - March 3, 2011, 9:38 a.m.
commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <kaber@trash.net>
Date:   Thu Mar 3 10:17:31 2011 +0100

    netlink: kill eff_cap from struct netlink_skb_parms
    
    Netlink message processing in the kernel is synchronous these days,
    capabilities can be checked directly in security_netlink_recv() from
    the current process.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>
James Morris - March 3, 2011, 10:49 a.m.
Reviewed-by: James Morris <jmorris@namei.org>
Chris Wright - March 3, 2011, 5:32 p.m.
* Patrick McHardy (kaber@trash.net) wrote:

> commit 8ff259625f0ab295fa085b0718eed13093813fbc
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Thu Mar 3 10:17:31 2011 +0100
> 
>     netlink: kill eff_cap from struct netlink_skb_parms
>     
>     Netlink message processing in the kernel is synchronous these days,
>     capabilities can be checked directly in security_netlink_recv() from
>     the current process.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Thanks for doing that Patrick.  I looked at this earlier and thought
there was still an async path, but I guess that's just to another
userspace process.

BTW, I think you missed a couple connector based callers:

drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_AD
drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))

Fix those and:

Acked-by: Chris Wright <chrisw@sous-sol.org>

Ideally, we'd consolidate those into a variant of security_netlink_recv().
However the issue is with types.  Inside connector callback we only have
netlink_skb_params (seems inapproriate to cast back out to skb).

We could change the lsm hook to only pass nsp, but SELinux actually
cares about the netlink type.  Any ideas?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 3, 2011, 6:56 p.m.
From: Chris Wright <chrisw@sous-sol.org>
Date: Thu, 3 Mar 2011 09:32:30 -0800

> * Patrick McHardy (kaber@trash.net) wrote:
> 
>> commit 8ff259625f0ab295fa085b0718eed13093813fbc
>> Author: Patrick McHardy <kaber@trash.net>
>> Date:   Thu Mar 3 10:17:31 2011 +0100
>> 
>>     netlink: kill eff_cap from struct netlink_skb_parms
>>     
>>     Netlink message processing in the kernel is synchronous these days,
>>     capabilities can be checked directly in security_netlink_recv() from
>>     the current process.
>>     
>>     Signed-off-by: Patrick McHardy <kaber@trash.net>
> 
> Thanks for doing that Patrick.  I looked at this earlier and thought
> there was still an async path, but I guess that's just to another
> userspace process.
> 
> BTW, I think you missed a couple connector based callers:
> 
> drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_AD
> drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> 
> Fix those and:
> 
> Acked-by: Chris Wright <chrisw@sous-sol.org>

Patrick, I'll apply your first patch, please respin this second patch with
the changes mentioned here.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Wright - March 3, 2011, 8:17 p.m.
* Chris Wright (chrisw@sous-sol.org) wrote:
> Ideally, we'd consolidate those into a variant of security_netlink_recv().
> However the issue is with types.  Inside connector callback we only have
> netlink_skb_params (seems inapproriate to cast back out to skb).
> 
> We could change the lsm hook to only pass nsp, but SELinux actually
> cares about the netlink type.  Any ideas?

Actually I misremembered, it only cares on the send path.

We could completely drop skb from recv lsm hook, will send an RFC
momentarily with example.

thanks,
-chris
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 8cbfaa6..fe81c85 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -2177,7 +2177,7 @@  static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms
 		return;
 	}
 
-	if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
+	if (!cap_raised(current_cap(), 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 049eaf1..1f23e04 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(nsp->eff_cap, CAP_SYS_ADMIN))
+	if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
 		return;
 
 	spin_lock(&receiving_list_lock);
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 66823b8..4c4ac3f 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -160,7 +160,6 @@  struct netlink_skb_parms {
 	struct ucred		creds;		/* Skb credentials	*/
 	__u32			pid;
 	__u32			dst_group;
-	kernel_cap_t		eff_cap;
 };
 
 #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 97ecd92..a808fb1 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1364,12 +1364,6 @@  static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 	NETLINK_CB(skb).dst_group = dst_group;
 	memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
 
-	/* What can I do? Netlink is asynchronous, so that
-	   we will have to save current capabilities to
-	   check them, when this message will be delivered
-	   to corresponding kernel module.   --ANK (980802)
-	 */
-
 	err = -EFAULT;
 	if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {
 		kfree_skb(skb);
diff --git a/security/commoncap.c b/security/commoncap.c
index 64c2ed9..a83e607 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -52,13 +52,12 @@  static void warn_setuid_and_fcaps_mixed(const char *fname)
 
 int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
-	NETLINK_CB(skb).eff_cap = current_cap();
 	return 0;
 }
 
 int cap_netlink_recv(struct sk_buff *skb, int cap)
 {
-	if (!cap_raised(NETLINK_CB(skb).eff_cap, cap))
+	if (!cap_raised(current_cap(), cap))
 		return -EPERM;
 	return 0;
 }