[PATCH 0/8] SECURITY ISSUE with connector

[PATCH 0/8] SECURITY ISSUE with connector

[Philipp Reisner]

Affected: All code that uses connector, in kernel and out of mainline

The connector, as it is today, does not allow the in kernel receiving
parts to do any checks on privileges of a message's sender.

I know, there are not many out there that like connector, but as
long as it is in the kernel, we have to fix the security issues it has!

Please either drop connector, or someone who feels a bit responsible
and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
this into your tree, and send the pull request to Linus.

Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
Patches 5 to 7 are the obvious fixes to the connector user's code.

For convenience these patches are also available as git tree:
git://git.drbd.org/linux-2.6-drbd.git connector-fix

-Phil

Philipp Reisner (8):
  connector: Keep the skb in cn_callback_data
  connector: Provide the sender's credentials to the callback
  connector/dm: Fixed a compilation warning
  connector: Removed the destruct_data callback since it is always kfree_skb()
  dm/connector: Only process connector packages from privileged processes
  dst/connector: Disallow unpliviged users to configure dst
  pohmelfs/connector: Disallow unpliviged users to configure pohmelfs
  uvesafb/connector: Disallow unpliviged users to send netlink packets

 Documentation/connector/cn_test.c      |    2 +-
 Documentation/connector/connector.txt  |    8 ++++----
 drivers/connector/cn_queue.c           |   12 +++++++-----
 drivers/connector/connector.c          |   22 ++++++++--------------
 drivers/md/dm-log-userspace-transfer.c |    6 ++++--
 drivers/staging/dst/dcore.c            |    7 ++++++-
 drivers/staging/pohmelfs/config.c      |    5 ++++-
 drivers/video/uvesafb.c                |    5 ++++-
 drivers/w1/w1_netlink.c                |    2 +-
 include/linux/connector.h              |   11 ++++-------
 10 files changed, 43 insertions(+), 37 deletions(-)

[PATCH 1/8] connector: Keep the skb in cn_callback_data

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Acked-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 drivers/connector/cn_queue.c  |    3 ++-
 drivers/connector/connector.c |   11 +++++------
 include/linux/connector.h     |    4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 4a1dfe1..b4cfac9 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -78,8 +78,9 @@ void cn_queue_wrapper(struct work_struct *work)
 	struct cn_callback_entry *cbq =
 		container_of(work, struct cn_callback_entry, work);
 	struct cn_callback_data *d = &cbq->data;
+	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(d->skb));
 
-	d->callback(d->callback_priv);
+	d->callback(msg);
 
 	d->destruct_data(d->ddata);
 	d->ddata = NULL;
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 74f52af..fc9887f 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -129,10 +129,11 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
 /*
  * Callback helper - queues work and setup destructor for given data.
  */
-static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), void *data)
+static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *), void *data)
 {
 	struct cn_callback_entry *__cbq, *__new_cbq;
 	struct cn_dev *dev = &cdev;
+	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(skb));
 	int err = -ENODEV;
 
 	spin_lock_bh(&dev->cbdev->queue_lock);
@@ -140,7 +141,7 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
 			if (likely(!work_pending(&__cbq->work) &&
 					__cbq->data.ddata == NULL)) {
-				__cbq->data.callback_priv = msg;
+				__cbq->data.skb = skb;
 
 				__cbq->data.ddata = data;
 				__cbq->data.destruct_data = destruct_data;
@@ -156,7 +157,7 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 				__new_cbq = kzalloc(sizeof(struct cn_callback_entry), GFP_ATOMIC);
 				if (__new_cbq) {
 					d = &__new_cbq->data;
-					d->callback_priv = msg;
+					d->skb = skb;
 					d->callback = __cbq->data.callback;
 					d->ddata = data;
 					d->destruct_data = destruct_data;
@@ -191,7 +192,6 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
  */
 static void cn_rx_skb(struct sk_buff *__skb)
 {
-	struct cn_msg *msg;
 	struct nlmsghdr *nlh;
 	int err;
 	struct sk_buff *skb;
@@ -208,8 +208,7 @@ static void cn_rx_skb(struct sk_buff *__skb)
 			return;
 		}
 
-		msg = NLMSG_DATA(nlh);
-		err = cn_call_callback(msg, (void (*)(void *))kfree_skb, skb);
+		err = cn_call_callback(skb, (void (*)(void *))kfree_skb, skb);
 		if (err < 0)
 			kfree_skb(skb);
 	}
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 47ebf41..05a7a14 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -134,8 +134,8 @@ struct cn_callback_id {
 struct cn_callback_data {
 	void (*destruct_data) (void *);
 	void *ddata;
-	
-	void *callback_priv;
+
+	struct sk_buff *skb;
 	void (*callback) (struct cn_msg *);
 
 	void *free;
-- 
1.6.0.4

[PATCH 2/8] connector: Provide the sender's credentials to the callback

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Acked-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 Documentation/connector/cn_test.c      |    2 +-
 Documentation/connector/connector.txt  |    8 ++++----
 drivers/connector/cn_queue.c           |    7 ++++---
 drivers/connector/connector.c          |    4 ++--
 drivers/md/dm-log-userspace-transfer.c |    2 +-
 drivers/staging/dst/dcore.c            |    2 +-
 drivers/staging/pohmelfs/config.c      |    2 +-
 drivers/video/uvesafb.c                |    2 +-
 drivers/w1/w1_netlink.c                |    2 +-
 include/linux/connector.h              |    6 +++---
 10 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/Documentation/connector/cn_test.c b/Documentation/connector/cn_test.c
index 1711adc..b07add3 100644
--- a/Documentation/connector/cn_test.c
+++ b/Documentation/connector/cn_test.c
@@ -34,7 +34,7 @@ static char cn_test_name[] = "cn_test";
 static struct sock *nls;
 static struct timer_list cn_test_timer;
 
-static void cn_test_callback(struct cn_msg *msg)
+static void cn_test_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	pr_info("%s: %lu: idx=%x, val=%x, seq=%u, ack=%u, len=%d: %s.\n",
 	        __func__, jiffies, msg->id.idx, msg->id.val,
diff --git a/Documentation/connector/connector.txt b/Documentation/connector/connector.txt
index 81e6bf6..78c9466 100644
--- a/Documentation/connector/connector.txt
+++ b/Documentation/connector/connector.txt
@@ -23,7 +23,7 @@ handling, etc...  The Connector driver allows any kernelspace agents to use
 netlink based networking for inter-process communication in a significantly
 easier way:
 
-int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *));
+int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 void cn_netlink_send(struct cn_msg *msg, u32 __group, int gfp_mask);
 
 struct cb_id
@@ -53,15 +53,15 @@ struct cn_msg
 Connector interfaces.
 /*****************************************/
 
-int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *));
+int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 
  Registers new callback with connector core.
 
  struct cb_id *id		- unique connector's user identifier.
 				  It must be registered in connector.h for legal in-kernel users.
  char *name			- connector's callback symbolic name.
- void (*callback) (void *)	- connector's callback.
-				  Argument must be dereferenced to struct cn_msg *.
+ void (*callback) (struct cn..)	- connector's callback.
+				  cn_msg and the sender's credentials
 
 
 void cn_del_callback(struct cb_id *id);
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index b4cfac9..163c3e3 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -79,8 +79,9 @@ void cn_queue_wrapper(struct work_struct *work)
 		container_of(work, struct cn_callback_entry, work);
 	struct cn_callback_data *d = &cbq->data;
 	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(d->skb));
+	struct netlink_skb_parms *nsp = &NETLINK_CB(d->skb);
 
-	d->callback(msg);
+	d->callback(msg, nsp);
 
 	d->destruct_data(d->ddata);
 	d->ddata = NULL;
@@ -90,7 +91,7 @@ void cn_queue_wrapper(struct work_struct *work)
 
 static struct cn_callback_entry *
 cn_queue_alloc_callback_entry(char *name, struct cb_id *id,
-			      void (*callback)(struct cn_msg *))
+			      void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	struct cn_callback_entry *cbq;
 
@@ -124,7 +125,7 @@ int cn_cb_equal(struct cb_id *i1, struct cb_id *i2)
 }
 
 int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id,
-			  void (*callback)(struct cn_msg *))
+			  void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	struct cn_callback_entry *cbq, *__cbq;
 	int found = 0;
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index fc9887f..e59f0ab 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -269,7 +269,7 @@ static void cn_notify(struct cb_id *id, u32 notify_event)
  * May sleep.
  */
 int cn_add_callback(struct cb_id *id, char *name,
-		    void (*callback)(struct cn_msg *))
+		    void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	int err;
 	struct cn_dev *dev = &cdev;
@@ -351,7 +351,7 @@ static int cn_ctl_msg_equals(struct cn_ctl_msg *m1, struct cn_ctl_msg *m2)
  *
  * Used for notification of a request's processing.
  */
-static void cn_callback(struct cn_msg *msg)
+static void cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct cn_ctl_msg *ctl;
 	struct cn_ctl_entry *ent;
diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index ba0edad..556131f 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -129,7 +129,7 @@ static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
  * This is the connector callback that delivers data
  * that was sent from userspace.
  */
-static void cn_ulog_callback(void *data)
+static void cn_ulog_callback(void *data, struct netlink_skb_parms *nsp)
 {
 	struct cn_msg *msg = (struct cn_msg *)data;
 	struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
diff --git a/drivers/staging/dst/dcore.c b/drivers/staging/dst/dcore.c
index ac85773..3943c91 100644
--- a/drivers/staging/dst/dcore.c
+++ b/drivers/staging/dst/dcore.c
@@ -847,7 +847,7 @@ static dst_command_func dst_commands[] = {
 /*
  * Configuration parser.
  */
-static void cn_dst_callback(struct cn_msg *msg)
+static void cn_dst_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct dst_ctl *ctl;
 	int err;
diff --git a/drivers/staging/pohmelfs/config.c b/drivers/staging/pohmelfs/config.c
index 90f962e..c9162b3 100644
--- a/drivers/staging/pohmelfs/config.c
+++ b/drivers/staging/pohmelfs/config.c
@@ -527,7 +527,7 @@ out_unlock:
 	return err;
 }
 
-static void pohmelfs_cn_callback(struct cn_msg *msg)
+static void pohmelfs_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	int err;
 
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index e98baf6..aa7cd95 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -67,7 +67,7 @@ static DEFINE_MUTEX(uvfb_lock);
  * find the kernel part of the task struct, copy the registers and
  * the buffer contents and then complete the task.
  */
-static void uvesafb_cn_callback(struct cn_msg *msg)
+static void uvesafb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct uvesafb_task *utask;
 	struct uvesafb_ktask *task;
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index 52ccb3d..45c126f 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -306,7 +306,7 @@ static int w1_netlink_send_error(struct cn_msg *rcmsg, struct w1_netlink_msg *rm
 	return error;
 }
 
-static void w1_cn_callback(struct cn_msg *msg)
+static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct w1_netlink_msg *m = (struct w1_netlink_msg *)(msg + 1);
 	struct w1_netlink_cmd *cmd;
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 05a7a14..545728e 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -136,7 +136,7 @@ struct cn_callback_data {
 	void *ddata;
 
 	struct sk_buff *skb;
-	void (*callback) (struct cn_msg *);
+	void (*callback) (struct cn_msg *, struct netlink_skb_parms *);
 
 	void *free;
 };
@@ -167,11 +167,11 @@ struct cn_dev {
 	struct cn_queue_dev *cbdev;
 };
 
-int cn_add_callback(struct cb_id *, char *, void (*callback) (struct cn_msg *));
+int cn_add_callback(struct cb_id *, char *, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 void cn_del_callback(struct cb_id *);
 int cn_netlink_send(struct cn_msg *, u32, gfp_t);
 
-int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id, void (*callback)(struct cn_msg *));
+int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id, void (*callback)(struct cn_msg *, struct netlink_skb_parms *));
 void cn_queue_del_callback(struct cn_queue_dev *dev, struct cb_id *id);
 
 int queue_cn_work(struct cn_callback_entry *cbq, struct work_struct *work);
-- 
1.6.0.4

[PATCH 3/8] connector/dm: Fixed a compilation warning

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Acked-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 drivers/md/dm-log-userspace-transfer.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index 556131f..1327e1a 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -129,9 +129,8 @@ static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
  * This is the connector callback that delivers data
  * that was sent from userspace.
  */
-static void cn_ulog_callback(void *data, struct netlink_skb_parms *nsp)
+static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = (struct cn_msg *)data;
 	struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
 
 	spin_lock(&receiving_list_lock);
-- 
1.6.0.4

[PATCH 4/8] connector: Removed the destruct_data callback since it is always kfree_skb()

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Acked-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 drivers/connector/cn_queue.c  |    4 ++--
 drivers/connector/connector.c |   11 +++--------
 include/linux/connector.h     |    3 ---
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 163c3e3..210338e 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -83,8 +83,8 @@ void cn_queue_wrapper(struct work_struct *work)
 
 	d->callback(msg, nsp);
 
-	d->destruct_data(d->ddata);
-	d->ddata = NULL;
+	kfree_skb(d->skb);
+	d->skb = NULL;
 
 	kfree(d->free);
 }
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index e59f0ab..f060246 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -129,7 +129,7 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
 /*
  * Callback helper - queues work and setup destructor for given data.
  */
-static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *), void *data)
+static int cn_call_callback(struct sk_buff *skb)
 {
 	struct cn_callback_entry *__cbq, *__new_cbq;
 	struct cn_dev *dev = &cdev;
@@ -140,12 +140,9 @@ static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *),
 	list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
 			if (likely(!work_pending(&__cbq->work) &&
-					__cbq->data.ddata == NULL)) {
+					__cbq->data.skb == NULL)) {
 				__cbq->data.skb = skb;
 
-				__cbq->data.ddata = data;
-				__cbq->data.destruct_data = destruct_data;
-
 				if (queue_cn_work(__cbq, &__cbq->work))
 					err = 0;
 				else
@@ -159,8 +156,6 @@ static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *),
 					d = &__new_cbq->data;
 					d->skb = skb;
 					d->callback = __cbq->data.callback;
-					d->ddata = data;
-					d->destruct_data = destruct_data;
 					d->free = __new_cbq;
 
 					__new_cbq->pdev = __cbq->pdev;
@@ -208,7 +203,7 @@ static void cn_rx_skb(struct sk_buff *__skb)
 			return;
 		}
 
-		err = cn_call_callback(skb, (void (*)(void *))kfree_skb, skb);
+		err = cn_call_callback(skb);
 		if (err < 0)
 			kfree_skb(skb);
 	}
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 545728e..3a14615 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -132,9 +132,6 @@ struct cn_callback_id {
 };
 
 struct cn_callback_data {
-	void (*destruct_data) (void *);
-	void *ddata;
-
 	struct sk_buff *skb;
 	void (*callback) (struct cn_msg *, struct netlink_skb_parms *);
 
-- 
1.6.0.4

[PATCH 5/8] dm/connector: Only process connector packages from privileged processes

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 drivers/md/dm-log-userspace-transfer.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index 1327e1a..54abf9e 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -133,6 +133,9 @@ 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))
+		return;
+
 	spin_lock(&receiving_list_lock);
 	if (msg->len == 0)
 		fill_pkg(msg, NULL);
-- 
1.6.0.4

[PATCH 6/8] dst/connector: Disallow unpliviged users to configure dst

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 drivers/staging/dst/dcore.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/dst/dcore.c b/drivers/staging/dst/dcore.c
index 3943c91..ee16010 100644
--- a/drivers/staging/dst/dcore.c
+++ b/drivers/staging/dst/dcore.c
@@ -855,6 +855,11 @@ static void cn_dst_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	struct dst_node *n = NULL, *tmp;
 	unsigned int hash;
 
+	if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
+		err = -EPERM;
+		goto out;
+	}
+
 	if (msg->len < sizeof(struct dst_ctl)) {
 		err = -EBADMSG;
 		goto out;
-- 
1.6.0.4

[PATCH 7/8] pohmelfs/connector: Disallow unpliviged users to configure pohmelfs

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 drivers/staging/pohmelfs/config.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/pohmelfs/config.c b/drivers/staging/pohmelfs/config.c
index c9162b3..5d04bf5 100644
--- a/drivers/staging/pohmelfs/config.c
+++ b/drivers/staging/pohmelfs/config.c
@@ -531,6 +531,9 @@ 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))
+		return;
+
 	switch (msg->flags) {
 		case POHMELFS_FLAGS_ADD:
 		case POHMELFS_FLAGS_DEL:
-- 
1.6.0.4

[PATCH 8/8] uvesafb/connector: Disallow unpliviged users to send netlink packets

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
---
 drivers/video/uvesafb.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index aa7cd95..e35232a 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -72,6 +72,9 @@ 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))
+		return;
+
 	if (msg->seq >= UVESAFB_TASKS_MAX)
 		return;
 
-- 
1.6.0.4

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Greg KH]

On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner wrote:
> Affected: All code that uses connector, in kernel and out of mainline
> 
> The connector, as it is today, does not allow the in kernel receiving
> parts to do any checks on privileges of a message's sender.

So, assume I know nothing about the connector architecture, what does
this mean in a security context?

> I know, there are not many out there that like connector, but as
> long as it is in the kernel, we have to fix the security issues it has!

And what specifically are the security issues?

> Please either drop connector, or someone who feels a bit responsible
> and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
> this into your tree, and send the pull request to Linus.
> 
> Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> Patches 5 to 7 are the obvious fixes to the connector user's code.

Obvious in what way?

thanks,

greg k-h

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Philipp Reisner]

> On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner wrote:
> > Affected: All code that uses connector, in kernel and out of mainline
> >
> > The connector, as it is today, does not allow the in kernel receiving
> > parts to do any checks on privileges of a message's sender.
>
> So, assume I know nothing about the connector architecture, what does
> this mean in a security context?
>

Think of the connector as a layer on top of netlink that allows more
than a hard coded number of subsystems to use netlink.

Netlink is used e.g. to modify routing tables in the kernel.

As it is today, subsystem utilising the connector can not examine
the capabilities of the user/program that sent the netlink message.

If the same would be true for netlink, than every unprivileged user
could change the routing tables on your box.

> > I know, there are not many out there that like connector, but as
> > long as it is in the kernel, we have to fix the security issues it has!
>
> And what specifically are the security issues?
>

unprivileged users can trigger operations that are supposed to be only
accessible to users having CAP_SYS_ADMIN (or some other CAP_XXX)

> > Please either drop connector, or someone who feels a bit responsible
> > and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take
> > this into your tree, and send the pull request to Linus.
> >
> > Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> > Patches 5 to 7 are the obvious fixes to the connector user's code.
>
> Obvious in what way?
>

They limit processing of connector/netlink messages in these subsystems
to messages sent from root (or some user having CAP_SYS_ADMIN).

That is obvious for dst, because device setup and destruction is done by
connector messages.

This is obvious for pohmelfs becuase these connector messages are
used there to change some configuration.

This is obvious for uvesafb because the connector messages are used 
there to delegate some video bios emulation to userspace. 

Last not least dm's dirty logging in user space, should be immune to
some crafted netlink packets sent by some unprivileged user.

Patches 1 to 4 fix the framework, should be merged as soon as possible.

Patches 5 to 8 (not 7) should probably be blessed by the affected
subsystem's maintainers. I think I have put all on CC. 

HTH.

-phil

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Greg KH]

On Fri, Oct 02, 2009 at 05:54:12PM +0200, Philipp Reisner wrote:
> > On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner wrote:
> > > Affected: All code that uses connector, in kernel and out of mainline
> > >
> > > The connector, as it is today, does not allow the in kernel receiving
> > > parts to do any checks on privileges of a message's sender.
> >
> > So, assume I know nothing about the connector architecture, what does
> > this mean in a security context?
> >
> 
> Think of the connector as a layer on top of netlink that allows more
> than a hard coded number of subsystems to use netlink.
> 
> Netlink is used e.g. to modify routing tables in the kernel.
> 
> As it is today, subsystem utilising the connector can not examine
> the capabilities of the user/program that sent the netlink message.
> 
> If the same would be true for netlink, than every unprivileged user
> could change the routing tables on your box.
> 
> > > I know, there are not many out there that like connector, but as
> > > long as it is in the kernel, we have to fix the security issues it has!
> >
> > And what specifically are the security issues?
> >
> 
> unprivileged users can trigger operations that are supposed to be only
> accessible to users having CAP_SYS_ADMIN (or some other CAP_XXX)

Ok, but it doesn't look like there are that many connector operations
right now, right?

Anyway, I have no objection to the patches, and figure they should go
through David's network tree.

thanks,

greg k-h

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Lars Ellenberg]

On Fri, Oct 02, 2009 at 06:58:59AM -0700, Greg KH wrote:
> On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner wrote:
> > Affected: All code that uses connector, in kernel and out of mainline
> > 
> > The connector, as it is today, does not allow the in kernel receiving
> > parts to do any checks on privileges of a message's sender.
> 
> So, assume I know nothing about the connector architecture, what does
> this mean in a security context?

Arbitrary unprivileged users may craft a netlink message, which gets delivered
through connector to callbacks (registered in kernel with cn_add_callback).

These callbacks will then act on the message, as if it originated from an
"expected" source.  But currently there is no mechanism to verify the origin, 
even if the callbacks would try to.

> > I know, there are not many out there that like connector, but as
> > long as it is in the kernel, we have to fix the security issues it has!
> 
> And what specifically are the security issues?

For the cn_ulog_callback (dm-log-userspace-transfer.c),
someone would be able to fake completion (with or without error code)
of ulog entries, copying arbitrary data into receiving_pkg entries.

   /*
    * This is the connector callback that delivers data
    * that was sent from userspace.
    */
   static void cn_ulog_callback(void *data)
   {
           struct cn_msg *msg = (struct cn_msg *)data;
           struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
   
           spin_lock(&receiving_list_lock);
           if (msg->len == 0)
                   fill_pkg(msg, NULL);
           else if (msg->len < sizeof(*tfr))
                   DMERR("Incomplete message received (expected %u, got %u): [%u]",
                         (unsigned)sizeof(*tfr), msg->len, msg->seq);
           else
                   fill_pkg(NULL, tfr);
           spin_unlock(&receiving_list_lock);
   }
   
   static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
   {
           uint32_t rtn_seq = (msg) ? msg->seq : (tfr) ? tfr->seq : 0;
   ...
                   } else {
                           pkg->error = tfr->error;
                           memcpy(pkg->data, tfr->data, tfr->data_size);
                           *(pkg->data_size) = tfr->data_size;
                   }
                   complete(&pkg->complete);

   
   
should make that obvious: if an unprivileged user can deliver arbitrary msg to
cn_ulog_callback, that should at least be disruptive to services that use it.

fix: check origin of message for proper credentials (e.g. CAP_SYS_ADMIN).




what or how much damage a crafted message can do in uvesafb_cn_callback,
I'm not sure. But, if I get the msg->seq right, and get by the first
sanity check, again, arbitrary input is copied into some
kernel object, which will likely at least confuse that subsystem,
maybe do damage, or result in some sort of denial of service.

I just don't know what these uvesafb_ktask do, but I doubt that anyone but root
should be able to manipulate them.




in the case of dst and pohemlfs, it is (re|de) configuration of respective in
kernel objects, possibly exposing arbitrary data content
	@Evgeniy - is that statement correct? Does something prevent an
	unprivileged user to export arbitrary things via dst?
At least some sort of denial of service should be possible there.

for DRBD, we have of course similar problems as long as we use the connector
in its current form as our configuration choice.



I'm not sure what actual harm can be done by arbitrary calling
w1_reset_select_slave(), or w1_process_command_io(),
but allowing unprivileged users to meddle with arbitrary devices is most likely
not the intended behaviour there, either.


The "obvious" way was to first make the credentials and capabilities of the
message origin available to these callbacks, and then test on "CAP_SYS_ADMIN".


Note that the suggested usage of the connector for _userspace_ tools
is to bind() to some netlink socket, subscribing to apropriate mutlicast
groups, which will usually fail for unprivileged users in netlink_bind()
because of

        /* Only superuser is allowed to listen multicasts */
        if (nladdr->nl_groups) {
                if (!netlink_capable(sock, NL_NONROOT_RECV))
                        return -EPERM;
                err = netlink_realloc_groups(sk);
                if (err)
                        return err;
        }

So typical userspace tools will fail when used as non-root.
But if you leave out the bind, you are perfectly able to _send_ arbitrary
messages on that socket, even if you are not able to receive any replies from
connector kernel space in that case.



Cheers,

	Lars

Re: [PATCH 0/8] SECURITY ISSUE with connector

[David Miller]

From: Philipp Reisner <philipp.reisner@linbit.com>
Date: Fri, 2 Oct 2009 17:54:12 +0200

> Think of the connector as a layer on top of netlink that allows more
> than a hard coded number of subsystems to use netlink.

There are no such limits in netlink, we have 'genetlink' which allows
an arbitrary number of subsystems to use netlink.

What connector provides over netlink/genetlink is something different
altogether.

Re: [PATCH 0/8] SECURITY ISSUE with connector

[David Miller]

From: Philipp Reisner <philipp.reisner@linbit.com>
Date: Fri,  2 Oct 2009 14:40:03 +0200

> Affected: All code that uses connector, in kernel and out of mainline
> 
> The connector, as it is today, does not allow the in kernel receiving
> parts to do any checks on privileges of a message's sender.
> 
> I know, there are not many out there that like connector, but as
> long as it is in the kernel, we have to fix the security issues it has!
> 
> Please either drop connector, or someone who feels a bit responsible
> and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
> this into your tree, and send the pull request to Linus.
> 
> Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> Patches 5 to 7 are the obvious fixes to the connector user's code.
> 
> For convenience these patches are also available as git tree:
> git://git.drbd.org/linux-2.6-drbd.git connector-fix

All applied to net-2.6, I'll push this out to Linus later
today.

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Greg KH]

On Fri, Oct 02, 2009 at 10:56:59AM -0700, David Miller wrote:
> From: Philipp Reisner <philipp.reisner@linbit.com>
> Date: Fri,  2 Oct 2009 14:40:03 +0200
> 
> > Affected: All code that uses connector, in kernel and out of mainline
> > 
> > The connector, as it is today, does not allow the in kernel receiving
> > parts to do any checks on privileges of a message's sender.
> > 
> > I know, there are not many out there that like connector, but as
> > long as it is in the kernel, we have to fix the security issues it has!
> > 
> > Please either drop connector, or someone who feels a bit responsible
> > and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
> > this into your tree, and send the pull request to Linus.
> > 
> > Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> > Patches 5 to 7 are the obvious fixes to the connector user's code.
> > 
> > For convenience these patches are also available as git tree:
> > git://git.drbd.org/linux-2.6-drbd.git connector-fix
> 
> All applied to net-2.6, I'll push this out to Linus later
> today.

Should it also go to -stable?  If so, I can pick it up once it hits
Linus's tree.

thanks,

greg k-h

Re: [PATCH 0/8] SECURITY ISSUE with connector

[David Miller]

From: Greg KH <greg@kroah.com>
Date: Fri, 2 Oct 2009 11:00:22 -0700

> On Fri, Oct 02, 2009 at 10:56:59AM -0700, David Miller wrote:
>> All applied to net-2.6, I'll push this out to Linus later
>> today.
> 
> Should it also go to -stable?  If so, I can pick it up once it hits
> Linus's tree.

Yes, please take it into -stable.

Greg, I'll also send you a batch of other networking bits
for -stable later this afternoon as well, just FYI...

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Greg KH]

On Fri, Oct 02, 2009 at 11:05:04AM -0700, David Miller wrote:
> From: Greg KH <greg@kroah.com>
> Date: Fri, 2 Oct 2009 11:00:22 -0700
> 
> > On Fri, Oct 02, 2009 at 10:56:59AM -0700, David Miller wrote:
> >> All applied to net-2.6, I'll push this out to Linus later
> >> today.
> > 
> > Should it also go to -stable?  If so, I can pick it up once it hits
> > Linus's tree.
> 
> Yes, please take it into -stable.

Will do.

> Greg, I'll also send you a batch of other networking bits
> for -stable later this afternoon as well, just FYI...

Great, I'll queue it up for the next -stable releases, these are full
enough as it is already :)

thanks,

greg k-h

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Evgeniy Polyakov]

On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner (philipp.reisner@linbit.com) wrote:
> Affected: All code that uses connector, in kernel and out of mainline
> 
> The connector, as it is today, does not allow the in kernel receiving
> parts to do any checks on privileges of a message's sender.
> 
> I know, there are not many out there that like connector, but as
> long as it is in the kernel, we have to fix the security issues it has!
> 
> Please either drop connector, or someone who feels a bit responsible
> and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
> this into your tree, and send the pull request to Linus.

How expressive! :)

> Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> Patches 5 to 7 are the obvious fixes to the connector user's code.

I ack those changes either since they do not affect logic of the user.

-- 
	Evgeniy Polyakov

Re: [PATCH 0/8] SECURITY ISSUE with connector

[Greg KH]

On Fri, Oct 02, 2009 at 02:40:03PM +0200, Philipp Reisner wrote:
> Affected: All code that uses connector, in kernel and out of mainline
> 
> The connector, as it is today, does not allow the in kernel receiving
> parts to do any checks on privileges of a message's sender.
> 
> I know, there are not many out there that like connector, but as
> long as it is in the kernel, we have to fix the security issues it has!
> 
> Please either drop connector, or someone who feels a bit responsible
> and has our beloved dictator's blessing, PLEASE PLEASE PLEASE take 
> this into your tree, and send the pull request to Linus.
> 
> Patches 1 to 4 are already Acked-by Evgeny, the connector's maintainer.
> Patches 5 to 7 are the obvious fixes to the connector user's code.

These don't apply to the 2.6.31-stable tree at all.

Could you provide them backported to that tree if you want to see them
go into a .31-stable release?

thanks,

greg k-h

[PATCH 0/7] SECURITY ISSUE with connector for 2.6.31.y

[Philipp Reisner]

The backported edition of the patchset for 2.6.31-stable.

Philipp Reisner (7):
  connector: Keep the skb in cn_callback_data
  connector: Provide the sender's credentials to the callback
  connector: Removed the destruct_data callback since it is always
    kfree_skb()
  dm/connector: Only process connector packages from privileged
    processes
  dst/connector: Disallow unpliviged users to configure dst
  pohmelfs/connector: Disallow unpliviged users to configure pohmelfs
  uvesafb/connector: Disallow unpliviged users to send netlink packets

 Documentation/connector/cn_test.c      |    4 +---
 Documentation/connector/connector.txt  |    8 ++++----
 drivers/connector/cn_proc.c            |    3 +--
 drivers/connector/cn_queue.c           |   15 ++++++++++-----
 drivers/connector/connector.c          |   24 +++++++++---------------
 drivers/md/dm-log-userspace-transfer.c |    6 ++++--
 drivers/staging/dst/dcore.c            |    8 ++++++--
 drivers/staging/pohmelfs/config.c      |    6 ++++--
 drivers/video/uvesafb.c                |    6 ++++--
 drivers/w1/w1_netlink.c                |    3 +--
 include/linux/connector.h              |   11 ++++-------
 11 files changed, 48 insertions(+), 46 deletions(-)

[PATCH 1/7] connector: Keep the skb in cn_callback_data

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
(cherry picked from commit 5491c43845dae6c68cb4edbcf2e2dde9a32a863d)
---
 drivers/connector/cn_queue.c  |    3 ++-
 drivers/connector/connector.c |   11 +++++------
 include/linux/connector.h     |    6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 408c2af..327dfde 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -78,8 +78,9 @@ void cn_queue_wrapper(struct work_struct *work)
 	struct cn_callback_entry *cbq =
 		container_of(work, struct cn_callback_entry, work);
 	struct cn_callback_data *d = &cbq->data;
+	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(d->skb));
 
-	d->callback(d->callback_priv);
+	d->callback(msg);
 
 	d->destruct_data(d->ddata);
 	d->ddata = NULL;
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 08b2500..9bb14ac 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -129,10 +129,11 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
 /*
  * Callback helper - queues work and setup destructor for given data.
  */
-static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), void *data)
+static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *), void *data)
 {
 	struct cn_callback_entry *__cbq, *__new_cbq;
 	struct cn_dev *dev = &cdev;
+	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(skb));
 	int err = -ENODEV;
 
 	spin_lock_bh(&dev->cbdev->queue_lock);
@@ -140,7 +141,7 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
 			if (likely(!work_pending(&__cbq->work) &&
 					__cbq->data.ddata == NULL)) {
-				__cbq->data.callback_priv = msg;
+				__cbq->data.skb = skb;
 
 				__cbq->data.ddata = data;
 				__cbq->data.destruct_data = destruct_data;
@@ -156,7 +157,7 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 				__new_cbq = kzalloc(sizeof(struct cn_callback_entry), GFP_ATOMIC);
 				if (__new_cbq) {
 					d = &__new_cbq->data;
-					d->callback_priv = msg;
+					d->skb = skb;
 					d->callback = __cbq->data.callback;
 					d->ddata = data;
 					d->destruct_data = destruct_data;
@@ -191,7 +192,6 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
  */
 static void cn_rx_skb(struct sk_buff *__skb)
 {
-	struct cn_msg *msg;
 	struct nlmsghdr *nlh;
 	int err;
 	struct sk_buff *skb;
@@ -208,8 +208,7 @@ static void cn_rx_skb(struct sk_buff *__skb)
 			return;
 		}
 
-		msg = NLMSG_DATA(nlh);
-		err = cn_call_callback(msg, (void (*)(void *))kfree_skb, skb);
+		err = cn_call_callback(skb, (void (*)(void *))kfree_skb, skb);
 		if (err < 0)
 			kfree_skb(skb);
 	}
diff --git a/include/linux/connector.h b/include/linux/connector.h
index b68d278..51b9ebd 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -134,9 +134,9 @@ struct cn_callback_id {
 struct cn_callback_data {
 	void (*destruct_data) (void *);
 	void *ddata;
-	
-	void *callback_priv;
-	void (*callback) (void *);
+
+	struct sk_buff *skb;
+	void (*callback) (struct cn_msg *);
 
 	void *free;
 };
-- 
1.6.0.4

[PATCH 2/7] connector: Provide the sender's credentials to the callback

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
---
 Documentation/connector/cn_test.c      |    4 +---
 Documentation/connector/connector.txt  |    8 ++++----
 drivers/connector/cn_proc.c            |    3 +--
 drivers/connector/cn_queue.c           |   10 +++++++---
 drivers/connector/connector.c          |    6 +++---
 drivers/md/dm-log-userspace-transfer.c |    3 +--
 drivers/staging/dst/dcore.c            |    3 +--
 drivers/staging/pohmelfs/config.c      |    3 +--
 drivers/video/uvesafb.c                |    3 +--
 drivers/w1/w1_netlink.c                |    3 +--
 include/linux/connector.h              |    6 +++---
 11 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/Documentation/connector/cn_test.c b/Documentation/connector/cn_test.c
index 6a5be5d..473c589 100644
--- a/Documentation/connector/cn_test.c
+++ b/Documentation/connector/cn_test.c
@@ -32,10 +32,8 @@ static char cn_test_name[] = "cn_test";
 static struct sock *nls;
 static struct timer_list cn_test_timer;
 
-void cn_test_callback(void *data)
+static void cn_test_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = (struct cn_msg *)data;
-
 	printk("%s: %lu: idx=%x, val=%x, seq=%u, ack=%u, len=%d: %s.\n",
 	       __func__, jiffies, msg->id.idx, msg->id.val,
 	       msg->seq, msg->ack, msg->len, (char *)msg->data);
diff --git a/Documentation/connector/connector.txt b/Documentation/connector/connector.txt
index ad6e0ba..3e6dcc7 100644
--- a/Documentation/connector/connector.txt
+++ b/Documentation/connector/connector.txt
@@ -23,7 +23,7 @@ handling...  Connector allows any kernelspace agents to use netlink
 based networking for inter-process communication in a significantly
 easier way:
 
-int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *));
+int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 void cn_netlink_send(struct cn_msg *msg, u32 __group, int gfp_mask);
 
 struct cb_id
@@ -53,15 +53,15 @@ struct cn_msg
 Connector interfaces.
 /*****************************************/
 
-int cn_add_callback(struct cb_id *id, char *name, void (*callback) (void *));
+int cn_add_callback(struct cb_id *id, char *name, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 
 Registers new callback with connector core.
 
 struct cb_id *id 		- unique connector's user identifier.
 			  	  It must be registered in connector.h for legal in-kernel users.
 char *name 			- connector's callback symbolic name.
-void (*callback) (void *)	- connector's callback.
-				  Argument must be dereferenced to struct cn_msg *.
+void (*callback) (struct cn..)	- connector's callback.
+				  cn_msg and the sender's credentials
 
 void cn_del_callback(struct cb_id *id);
 
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index c5afc98..9ca20d0 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -202,9 +202,8 @@ static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
  * cn_proc_mcast_ctl
  * @data: message sent from userspace via the connector
  */
-static void cn_proc_mcast_ctl(void *data)
+static void cn_proc_mcast_ctl(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = data;
 	enum proc_cn_mcast_op *mc_op = NULL;
 	int err = 0;
 
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 327dfde..163c3e3 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -79,8 +79,9 @@ void cn_queue_wrapper(struct work_struct *work)
 		container_of(work, struct cn_callback_entry, work);
 	struct cn_callback_data *d = &cbq->data;
 	struct cn_msg *msg = NLMSG_DATA(nlmsg_hdr(d->skb));
+	struct netlink_skb_parms *nsp = &NETLINK_CB(d->skb);
 
-	d->callback(msg);
+	d->callback(msg, nsp);
 
 	d->destruct_data(d->ddata);
 	d->ddata = NULL;
@@ -88,7 +89,9 @@ void cn_queue_wrapper(struct work_struct *work)
 	kfree(d->free);
 }
 
-static struct cn_callback_entry *cn_queue_alloc_callback_entry(char *name, struct cb_id *id, void (*callback)(void *))
+static struct cn_callback_entry *
+cn_queue_alloc_callback_entry(char *name, struct cb_id *id,
+			      void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	struct cn_callback_entry *cbq;
 
@@ -121,7 +124,8 @@ int cn_cb_equal(struct cb_id *i1, struct cb_id *i2)
 	return ((i1->idx == i2->idx) && (i1->val == i2->val));
 }
 
-int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id, void (*callback)(void *))
+int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id,
+			  void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	struct cn_callback_entry *cbq, *__cbq;
 	int found = 0;
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 9bb14ac..e59f0ab 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -268,7 +268,8 @@ static void cn_notify(struct cb_id *id, u32 notify_event)
  *
  * May sleep.
  */
-int cn_add_callback(struct cb_id *id, char *name, void (*callback)(void *))
+int cn_add_callback(struct cb_id *id, char *name,
+		    void (*callback)(struct cn_msg *, struct netlink_skb_parms *))
 {
 	int err;
 	struct cn_dev *dev = &cdev;
@@ -350,9 +351,8 @@ static int cn_ctl_msg_equals(struct cn_ctl_msg *m1, struct cn_ctl_msg *m2)
  *
  * Used for notification of a request's processing.
  */
-static void cn_callback(void *data)
+static void cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = data;
 	struct cn_ctl_msg *ctl;
 	struct cn_ctl_entry *ent;
 	u32 size;
diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index ba0edad..1327e1a 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -129,9 +129,8 @@ static int fill_pkg(struct cn_msg *msg, struct dm_ulog_request *tfr)
  * This is the connector callback that delivers data
  * that was sent from userspace.
  */
-static void cn_ulog_callback(void *data)
+static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = (struct cn_msg *)data;
 	struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
 
 	spin_lock(&receiving_list_lock);
diff --git a/drivers/staging/dst/dcore.c b/drivers/staging/dst/dcore.c
index fad25b7..32f102d 100644
--- a/drivers/staging/dst/dcore.c
+++ b/drivers/staging/dst/dcore.c
@@ -846,10 +846,9 @@ static dst_command_func dst_commands[] = {
 /*
  * Configuration parser.
  */
-static void cn_dst_callback(void *data)
+static void cn_dst_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
 	struct dst_ctl *ctl;
-	struct cn_msg *msg = data;
 	int err;
 	struct dst_ctl_ack ack;
 	struct dst_node *n = NULL, *tmp;
diff --git a/drivers/staging/pohmelfs/config.c b/drivers/staging/pohmelfs/config.c
index a6eaa42..1b4d564 100644
--- a/drivers/staging/pohmelfs/config.c
+++ b/drivers/staging/pohmelfs/config.c
@@ -446,9 +446,8 @@ out_unlock:
 	return err;
 }
 
-static void pohmelfs_cn_callback(void *data)
+static void pohmelfs_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = data;
 	int err;
 
 	switch (msg->flags) {
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index ca5b464..aa7cd95 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -67,9 +67,8 @@ static DEFINE_MUTEX(uvfb_lock);
  * find the kernel part of the task struct, copy the registers and
  * the buffer contents and then complete the task.
  */
-static void uvesafb_cn_callback(void *data)
+static void uvesafb_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = data;
 	struct uvesafb_task *utask;
 	struct uvesafb_ktask *task;
 
diff --git a/drivers/w1/w1_netlink.c b/drivers/w1/w1_netlink.c
index fdf7285..45c126f 100644
--- a/drivers/w1/w1_netlink.c
+++ b/drivers/w1/w1_netlink.c
@@ -306,9 +306,8 @@ static int w1_netlink_send_error(struct cn_msg *rcmsg, struct w1_netlink_msg *rm
 	return error;
 }
 
-static void w1_cn_callback(void *data)
+static void w1_cn_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 {
-	struct cn_msg *msg = data;
 	struct w1_netlink_msg *m = (struct w1_netlink_msg *)(msg + 1);
 	struct w1_netlink_cmd *cmd;
 	struct w1_slave *sl;
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 51b9ebd..545728e 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -136,7 +136,7 @@ struct cn_callback_data {
 	void *ddata;
 
 	struct sk_buff *skb;
-	void (*callback) (struct cn_msg *);
+	void (*callback) (struct cn_msg *, struct netlink_skb_parms *);
 
 	void *free;
 };
@@ -167,11 +167,11 @@ struct cn_dev {
 	struct cn_queue_dev *cbdev;
 };
 
-int cn_add_callback(struct cb_id *, char *, void (*callback) (void *));
+int cn_add_callback(struct cb_id *, char *, void (*callback) (struct cn_msg *, struct netlink_skb_parms *));
 void cn_del_callback(struct cb_id *);
 int cn_netlink_send(struct cn_msg *, u32, gfp_t);
 
-int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id, void (*callback)(void *));
+int cn_queue_add_callback(struct cn_queue_dev *dev, char *name, struct cb_id *id, void (*callback)(struct cn_msg *, struct netlink_skb_parms *));
 void cn_queue_del_callback(struct cn_queue_dev *dev, struct cb_id *id);
 
 int queue_cn_work(struct cn_callback_entry *cbq, struct work_struct *work);
-- 
1.6.0.4

[PATCH 3/7] connector: Removed the destruct_data callback since it is always kfree_skb()

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
Acked-by: Lars Ellenberg <lars.ellenberg@linbit.com>
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
(cherry picked from commit f4b5129f5e838942f759c2637967441cf4a98c20)
---
 drivers/connector/cn_queue.c  |    4 ++--
 drivers/connector/connector.c |   11 +++--------
 include/linux/connector.h     |    3 ---
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 163c3e3..210338e 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -83,8 +83,8 @@ void cn_queue_wrapper(struct work_struct *work)
 
 	d->callback(msg, nsp);
 
-	d->destruct_data(d->ddata);
-	d->ddata = NULL;
+	kfree_skb(d->skb);
+	d->skb = NULL;
 
 	kfree(d->free);
 }
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index e59f0ab..f060246 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -129,7 +129,7 @@ EXPORT_SYMBOL_GPL(cn_netlink_send);
 /*
  * Callback helper - queues work and setup destructor for given data.
  */
-static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *), void *data)
+static int cn_call_callback(struct sk_buff *skb)
 {
 	struct cn_callback_entry *__cbq, *__new_cbq;
 	struct cn_dev *dev = &cdev;
@@ -140,12 +140,9 @@ static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *),
 	list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
 			if (likely(!work_pending(&__cbq->work) &&
-					__cbq->data.ddata == NULL)) {
+					__cbq->data.skb == NULL)) {
 				__cbq->data.skb = skb;
 
-				__cbq->data.ddata = data;
-				__cbq->data.destruct_data = destruct_data;
-
 				if (queue_cn_work(__cbq, &__cbq->work))
 					err = 0;
 				else
@@ -159,8 +156,6 @@ static int cn_call_callback(struct sk_buff *skb, void (*destruct_data)(void *),
 					d = &__new_cbq->data;
 					d->skb = skb;
 					d->callback = __cbq->data.callback;
-					d->ddata = data;
-					d->destruct_data = destruct_data;
 					d->free = __new_cbq;
 
 					__new_cbq->pdev = __cbq->pdev;
@@ -208,7 +203,7 @@ static void cn_rx_skb(struct sk_buff *__skb)
 			return;
 		}
 
-		err = cn_call_callback(skb, (void (*)(void *))kfree_skb, skb);
+		err = cn_call_callback(skb);
 		if (err < 0)
 			kfree_skb(skb);
 	}
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 545728e..3a14615 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -132,9 +132,6 @@ struct cn_callback_id {
 };
 
 struct cn_callback_data {
-	void (*destruct_data) (void *);
-	void *ddata;
-
 	struct sk_buff *skb;
 	void (*callback) (struct cn_msg *, struct netlink_skb_parms *);
 
-- 
1.6.0.4

[PATCH 4/7] dm/connector: Only process connector packages from privileged processes

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
(cherry picked from commit 93136335f9ad7a98b92eacda1b43dccbf063cd07)
---
 drivers/md/dm-log-userspace-transfer.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index 1327e1a..54abf9e 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -133,6 +133,9 @@ 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))
+		return;
+
 	spin_lock(&receiving_list_lock);
 	if (msg->len == 0)
 		fill_pkg(msg, NULL);
-- 
1.6.0.4

[PATCH 5/7] dst/connector: Disallow unpliviged users to configure dst

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
(cherry picked from commit dbbb3431228784612848a1ec6061c78b4b708b5c)
---
 drivers/staging/dst/dcore.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/dst/dcore.c b/drivers/staging/dst/dcore.c
index 32f102d..5546898 100644
--- a/drivers/staging/dst/dcore.c
+++ b/drivers/staging/dst/dcore.c
@@ -854,6 +854,11 @@ static void cn_dst_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
 	struct dst_node *n = NULL, *tmp;
 	unsigned int hash;
 
+	if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
+		err = -EPERM;
+		goto out;
+	}
+
 	if (msg->len < sizeof(struct dst_ctl)) {
 		err = -EBADMSG;
 		goto out;
-- 
1.6.0.4

[PATCH 6/7] pohmelfs/connector: Disallow unpliviged users to configure pohmelfs

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
(cherry picked from commit 0179065b13b354cc0b940e7a632a65ec0448beff)
---
 drivers/staging/pohmelfs/config.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/pohmelfs/config.c b/drivers/staging/pohmelfs/config.c
index 1b4d564..d8ec47a 100644
--- a/drivers/staging/pohmelfs/config.c
+++ b/drivers/staging/pohmelfs/config.c
@@ -450,6 +450,9 @@ 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))
+		return;
+
 	switch (msg->flags) {
 		case POHMELFS_FLAGS_ADD:
 		case POHMELFS_FLAGS_DEL:
-- 
1.6.0.4

[PATCH 7/7] uvesafb/connector: Disallow unpliviged users to send netlink packets

[Philipp Reisner]

Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com>
(cherry picked from commit 30efa3f76813b17445bc5a2e443ae9731518566b)
---
 drivers/video/uvesafb.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index aa7cd95..e35232a 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -72,6 +72,9 @@ 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))
+		return;
+
 	if (msg->seq >= UVESAFB_TASKS_MAX)
 		return;
 
-- 
1.6.0.4

Re: [PATCH 0/7] SECURITY ISSUE with connector for 2.6.31.y

[Serge E. Hallyn]

Quoting Philipp Reisner (philipp.reisner@linbit.com):
> The backported edition of the patchset for 2.6.31-stable.
> 
> Philipp Reisner (7):
>   connector: Keep the skb in cn_callback_data
>   connector: Provide the sender's credentials to the callback
>   connector: Removed the destruct_data callback since it is always
>     kfree_skb()
>   dm/connector: Only process connector packages from privileged
>     processes
>   dst/connector: Disallow unpliviged users to configure dst
>   pohmelfs/connector: Disallow unpliviged users to configure pohmelfs
>   uvesafb/connector: Disallow unpliviged users to send netlink packets

Thanks Philipp, I see it's already applied upstream, but it looks good to me.
Does drivers/w1/w1_netlink.c or drivers/connector/cn_proc.c need a caps check
added as well?

thanks,
-serge

Re: [PATCH 0/7] SECURITY ISSUE with connector for 2.6.31.y

[Greg KH]

On Tue, Oct 13, 2009 at 11:28:11AM +0200, Philipp Reisner wrote:
> The backported edition of the patchset for 2.6.31-stable.
> 
> Philipp Reisner (7):
>   connector: Keep the skb in cn_callback_data
>   connector: Provide the sender's credentials to the callback
>   connector: Removed the destruct_data callback since it is always
>     kfree_skb()
>   dm/connector: Only process connector packages from privileged
>     processes
>   dst/connector: Disallow unpliviged users to configure dst
>   pohmelfs/connector: Disallow unpliviged users to configure pohmelfs
>   uvesafb/connector: Disallow unpliviged users to send netlink packets

All queued up, thanks.

greg k-h