| Message ID | 20110303201522.GT4988@sequoia.sous-sol.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Chris Wright <chrisw@sous-sol.org> Date: Thu, 3 Mar 2011 12:15:22 -0800 > Here, I respun it so I could work on top of it ... > I did not do exhaustive .config compile tests Thanks a lot Chris, applied. -- 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
On Thu, Mar 03, 2011 at 12:15:22PM -0800, Chris Wright wrote: > * David Miller (davem@davemloft.net) wrote: > > 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)) 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 -- 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
* Lars Ellenberg (lars.ellenberg@linbit.com) 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. > 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. -- 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
Hi. On Thu, Mar 03, 2011 at 11:37:46PM +0100, Lars Ellenberg (lars.ellenberg@linbit.com) wrote: > 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.
From: Evgeniy Polyakov <zbr@ioremap.net> Date: Fri, 4 Mar 2011 04:29:56 +0300 > Hi. > > On Thu, Mar 03, 2011 at 11:37:46PM +0100, Lars Ellenberg (lars.ellenberg@linbit.com) wrote: >> 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. Yes, please it would really help us with what we're trying to do here. -- 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
Am 04.03.2011 02:29, schrieb Evgeniy Polyakov: > Hi. > > On Thu, Mar 03, 2011 at 11:37:46PM +0100, Lars Ellenberg (lars.ellenberg@linbit.com) wrote: >> 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. > Are you going to do this or do you want me to take care of it? -- 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
Hi Patrick. On Tue, Mar 08, 2011 at 03:50:47PM +0100, Patrick McHardy (kaber@trash.net) wrote: > > 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 :)
Am 08.03.2011 19:32, schrieb Evgeniy Polyakov: > Hi Patrick. > > On Tue, Mar 08, 2011 at 03:50:47PM +0100, Patrick McHardy (kaber@trash.net) wrote: >>> 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 :) Thanks Evgeniy, I'll give it a shot. -- 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
Hi. On Tue, Mar 08, 2011 at 07:54:33PM +0100, Patrick McHardy (kaber@trash.net) wrote: > >> 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?
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/drivers/staging/pohmelfs/config.c b/drivers/staging/pohmelfs/config.c index 89279ba..39413b7 100644 --- a/drivers/staging/pohmelfs/config.c +++ b/drivers/staging/pohmelfs/config.c @@ -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) { diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c index 52ec095..5180a21 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(nsp->eff_cap, CAP_SYS_ADMIN)) + if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) return; if (msg->seq >= UVESAFB_TASKS_MAX) 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; }