netlink: kill eff_cap from struct netlink_skb_parms

PATCH In review Patrick McHardy 6 participants 15 messages Mar 3, 2011 – Mar 17, 2011 1 message not in the archive

Subsystems (9)

Networking Network drivers Staging Block layer +5 more subsystems

lore

Share & tools

lore

Patrick McHardy

author

2011-03-03 09:38 UTC #0 lore raw

PATCH 2/2 netlink: kill eff_cap from struct netlink_skb_parms

commit 8ff259625f0ab295fa085b0718eed13093813fbc

Author: Patrick McHardy <[email protected]>
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 <[email protected]>

drivers/block/drbd/drbd_nl.c +1 -1

index 8cbfaa6..fe81c85 100644
@@ -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;
 	}

drivers/md/dm-log-userspace-transfer.c +1 -1

index 049eaf1..1f23e04 100644
@@ -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);

include/linux/netlink.h +0 -1

index 66823b8..4c4ac3f 100644
@@ -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))

net/netlink/af_netlink.c +0 -6

index 97ecd92..a808fb1 100644
@@ -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);

security/commoncap.c +1 -2

index 64c2ed9..a83e607 100644
@@ -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;
 }

James Morris

maintainer Reviewed-by on v1 2/2

2011-03-03 10:49 UTC #1 lore raw

Reviewed-by James Morris <[email protected]>

Signature · 2 lines

James Morris
<[email protected]>

Chris Wright

Acked-by on v1 2/2

2011-03-03 17:32 UTC #2 lore raw

In reply to Patrick McHardy

Quoted 11 lines · * Patrick McHardy ([email protected]) wrote:

commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <[email protected]>
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 <[email protected]>

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 <[email protected]>

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?

David Miller

maintainer

2011-03-03 18:56 UTC #3 lore raw

From: Chris Wright <[email protected]>
Date: Thu, 3 Mar 2011 09:32:30 -0800

Quoted 18 lines · * Patrick McHardy ([email protected]) wrote:

Quoted 5 lines
commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <[email protected]>
Date:   Thu Mar 3 10:17:31 2011 +0100
Quoted 7 lines
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 <[email protected]>
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.

Quoted 8 lines

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 <[email protected]>

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

Thanks!

Chris Wright

2011-03-03 20:15 UTC #4 lore raw

PATCH v2 2/2 netlink: kill eff_cap from struct netlink_skb_parms

Quoted 26 lines · * David Miller ([email protected]) wrote:

From: Chris Wright <[email protected]>
Date: Thu, 3 Mar 2011 09:32:30 -0800
Quoted 23 lines · * Patrick McHardy ([email protected]) wrote:
Quoted 5 lines
commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <[email protected]>
Date:   Thu Mar 3 10:17:31 2011 +0100
Quoted 7 lines
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 <[email protected]>
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))

Quoted 6 lines

Quoted 3 lines
Fix those and:
Acked-by Chris Wright <[email protected]>
Patrick, I'll apply your first patch, please respin this second patch with the changes mentioned here.

Here, I respun it so I could work on top of it

thanks,
-chris
---
From: Patrick McHardy <[email protected]>
Subject: [PATCH 2/2 v2] 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 <[email protected]>

Reviewed-by James Morris <[email protected]>

[chrisw: update to include pohmelfs and uvesafb]

Signed-off-by Chris Wright <[email protected]>

---

I did not do exhaustive .config compile tests

 drivers/block/drbd/drbd_nl.c           |    2 +-
 drivers/md/dm-log-userspace-transfer.c |    2 +-
 drivers/staging/pohmelfs/config.c      |    2 +-
 drivers/video/uvesafb.c                |    2 +-
 include/linux/netlink.h                |    1 -
 net/netlink/af_netlink.c               |    6 ------
 security/commoncap.c                   |    3 +--
 7 files changed, 5 insertions(+), 13 deletions(-)

drivers/block/drbd/drbd_nl.c +1 -1

index 8cbfaa6..fe81c85 100644
@@ -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;
 	}

drivers/md/dm-log-userspace-transfer.c +1 -1

index 049eaf1..1f23e04 100644
@@ -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);

drivers/staging/pohmelfs/config.c +1 -1

index 89279ba..39413b7 100644
@@ -525,7 +525,7 @@ static void pohmelfs_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *n
 {
 	int err;
 
-	if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
+	if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
 		return;
 
 	switch (msg->flags) {

drivers/video/uvesafb.c +1 -1

index 52ec095..5180a21 100644
@@ -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(nsp->eff_cap, CAP_SYS_ADMIN))
+	if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
 		return;
 
 	if (msg->seq >= UVESAFB_TASKS_MAX)

include/linux/netlink.h +0 -1

index 66823b8..4c4ac3f 100644
@@ -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))

net/netlink/af_netlink.c +0 -6

index 97ecd92..a808fb1 100644
@@ -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);

security/commoncap.c +1 -2

index 64c2ed9..a83e607 100644
@@ -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;
 }

Signature · 1 line

1.7.1

David Miller

maintainer

2011-03-03 21:39 UTC #5 lore raw

From: Chris Wright <[email protected]>
Date: Thu, 3 Mar 2011 12:15:22 -0800

Quoted 1 line

Here, I respun it so I could work on top of it

...

Quoted 1 line

I did not do exhaustive .config compile tests

Thanks a lot Chris, applied.

A message that is not in the archive (sent off-list or not yet received): 20110303201522.GT4988-JyIX8gxvWYPr2PDY2+4mTGD2FQJk+8+b@public.gmane.org

Lars Ellenberg

2011-03-03 22:37 UTC #7 lore raw

Quoted 18 lines · Chris Wright

Quoted 17 lines · * David Miller (davem-fT/PcQaiUtIeIZ0/[email protected]) wrote:
From: Chris Wright <[email protected]>
Date: Thu, 3 Mar 2011 09:32:30 -0800
Quoted 14 lines · * Patrick McHardy ([email protected]) wrote:
Quoted 12 lines
commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <[email protected]>
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 <[email protected]>

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
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))

Last time I checked, current() for connector based netlink message consumers was the work queue that is used for connector.

So unless that changed, or my understanding is wrong, current_cap() inside cn_queue_wrapper(), respectively the d->callback() will not be the userland sender process' capabilities, but the work queue capabilities.

If so, then this change introduces the possibility for normal users to send privileged commands to connector based subsystems, even if they may not be able to bind() to suitable sockets to receive any replies.

Am I missing something?

Lars

Chris Wright

2011-03-03 23:53 UTC #8 lore raw

Quoted 7 lines · * Lars Ellenberg ([email protected]) wrote:

Last time I checked, current() for connector based netlink message consumers was the work queue that is used for connector.
So unless that changed, or my understanding is wrong, current_cap() inside cn_queue_wrapper(), respectively the d->callback() will not be the userland sender process' capabilities, but the work queue capabilities.

Yes, you're right.

Quoted 5 lines

If so, then this change introduces the possibility for normal users to send privileged commands to connector based subsystems, even if they may not be able to bind() to suitable sockets to receive any replies.
Am I missing something?

No, thanks for review. This puts back the async issue.

Evgeniy Polyakov

2011-03-04 01:29 UTC #9 lore raw

In reply to Lars Ellenberg

Hi.

Quoted 5 lines · Lars Ellenberg

If so, then this change introduces the possibility for normal users to send privileged commands to connector based subsystems, even if they may not be able to bind() to suitable sockets to receive any replies.
Am I missing something?

Yup, connector is very async at that place, but I wonder why the hell I ever made that decision. I believe we can replace it with pure sync call of the registered connector callback, since netlink is synchronous and no one has any problem with it.

Signature · 1 line

	Evgeniy Polyakov

David Miller

maintainer

2011-03-04 01:38 UTC #10 lore raw

From: Evgeniy Polyakov <[email protected]>
Date: Fri, 4 Mar 2011 04:29:56 +0300

Quoted 7 lines

Hi.
Quoted 4 lines · Lars Ellenberg
If so, then this change introduces the possibility for normal users to send privileged commands to connector based subsystems, even if they may not be able to bind() to suitable sockets to receive any replies.

Quoted 6 lines

Quoted 1 line
Am I missing something?
Yup, connector is very async at that place, but I wonder why the hell I ever made that decision. I believe we can replace it with pure sync call of the registered connector callback, since netlink is synchronous and no one has any problem with it.

Yes, please it would really help us with what we're trying to do here.

Patrick McHardy

author

2011-03-08 14:50 UTC #11 lore raw

In reply to Evgeniy Polyakov

Am 04.03.2011 02:29, schrieb Evgeniy Polyakov:

Quoted 7 lines

Hi.
Quoted 4 lines · Lars Ellenberg
If so, then this change introduces the possibility for normal users to send privileged commands to connector based subsystems, even if they may not be able to bind() to suitable sockets to receive any replies.

Quoted 7 lines

Quoted 1 line
Am I missing something?
Yup, connector is very async at that place, but I wonder why the hell I ever made that decision. I believe we can replace it with pure sync call of the registered connector callback, since netlink is synchronous and no one has any problem with it.

Are you going to do this or do you want me to take care of it?

Evgeniy Polyakov

2011-03-08 18:32 UTC #12 lore raw

Hi Patrick.

Quoted 6 lines · Patrick McHardy

Quoted 4 lines
Yup, connector is very async at that place, but I wonder why the hell I ever made that decision. I believe we can replace it with pure sync call of the registered connector callback, since netlink is synchronous and no one has any problem with it.
Are you going to do this or do you want me to take care of it?

I will return back at the end of the week and will take care of this problem. I will not mind if you complete it first though :)

Signature · 1 line

	Evgeniy Polyakov

Patrick McHardy

author

2011-03-08 18:54 UTC #13 lore raw

Am 08.03.2011 19:32, schrieb Evgeniy Polyakov:

Quoted 8 lines

Hi Patrick.
Quoted 5 lines · Patrick McHardy
Quoted 4 lines
Yup, connector is very async at that place, but I wonder why the hell I ever made that decision. I believe we can replace it with pure sync call of the registered connector callback, since netlink is synchronous and no one has any problem with it.

Quoted 4 lines

Quoted 1 line
Are you going to do this or do you want me to take care of it?
I will return back at the end of the week and will take care of this problem. I will not mind if you complete it first though :)

Thanks Evgeniy, I'll give it a shot.

Evgeniy Polyakov

2011-03-17 15:43 UTC #14 lore raw

Hi.

Quoted 6 lines · Patrick McHardy

Quoted 4 lines
Quoted 1 line
Are you going to do this or do you want me to take care of it?
I will return back at the end of the week and will take care of this problem. I will not mind if you complete it first though :)
Thanks Evgeniy, I'll give it a shot.

Is my help needed there or you will clean things up?

Signature · 1 line

	Evgeniy Polyakov

Chris Wright

2011-03-03 20:17 UTC #15 lore raw

In reply to Chris Wright

Quoted 6 lines · * Chris Wright ([email protected]) 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