diff mbox series

netlink: fix policy dump leak

Message ID 20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid
State New
Headers show
Series netlink: fix policy dump leak | expand

Commit Message

Johannes Berg Oct. 2, 2020, 7:46 a.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

If userspace doesn't complete the policy dump, we leak the
allocated state. Fix this.

Fixes: d07dcf9aadd6 ("netlink: add infrastructure to expose policies to userspace")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Found this while looking at Jakub's series and the complete op dump
that I said I'd do ...

Jakub, this conflicts with your series now, of course. Not sure how
we want to handle that?
---
 include/net/netlink.h   |  3 ++-
 net/netlink/genetlink.c |  9 ++++++++-
 net/netlink/policy.c    | 24 ++++++++++--------------
 3 files changed, 20 insertions(+), 16 deletions(-)

Comments

Jakub Kicinski Oct. 2, 2020, 3:29 p.m. UTC | #1 
On Fri,  2 Oct 2020 09:46:04 +0200 Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If userspace doesn't complete the policy dump, we leak the
> allocated state. Fix this.
> 
> Fixes: d07dcf9aadd6 ("netlink: add infrastructure to expose policies to userspace")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

FWIW make sure to mark the patches with net and net-next in the tag. 
The more automation we have, the more it matters.
Johannes Berg Oct. 2, 2020, 3:30 p.m. UTC | #2 
On Fri, 2020-10-02 at 08:29 -0700, Jakub Kicinski wrote:
> On Fri,  2 Oct 2020 09:46:04 +0200 Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > If userspace doesn't complete the policy dump, we leak the
> > allocated state. Fix this.
> > 
> > Fixes: d07dcf9aadd6 ("netlink: add infrastructure to expose policies to userspace")
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks.

> FWIW make sure to mark the patches with net and net-next in the tag. 
> The more automation we have, the more it matters.

Yeah, sorry. It occurred to me like 10 seconds after sending the patch,
and then of course I forgot _again_ when I sent the others ... I'm not
doing that (yet) on my trees, so not quite natural yet.

johannes
David Miller Oct. 2, 2020, 10:22 p.m. UTC | #3 
From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri,  2 Oct 2020 09:46:04 +0200

> From: Johannes Berg <johannes.berg@intel.com>
> 
> If userspace doesn't complete the policy dump, we leak the
> allocated state. Fix this.
> 
> Fixes: d07dcf9aadd6 ("netlink: add infrastructure to expose policies to userspace")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Applied and queued up for -stable, thanks.

> Jakub, this conflicts with your series now, of course. Not sure how
> we want to handle that?

I applied this to both net and net-next in order to facilitate Jakub's
work.
diff mbox series

Patch

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 8e0eb2c9c528..271620f6bc7f 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1934,7 +1934,8 @@  void nla_get_range_signed(const struct nla_policy *pt,
 int netlink_policy_dump_start(const struct nla_policy *policy,
 			      unsigned int maxtype,
 			      unsigned long *state);
-bool netlink_policy_dump_loop(unsigned long *state);
+bool netlink_policy_dump_loop(unsigned long state);
 int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);
+void netlink_policy_dump_free(unsigned long state);
 
 #endif
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 1eb65a7a27fd..c4b4d3376227 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1079,7 +1079,7 @@  static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 	if (err)
 		return err;
 
-	while (netlink_policy_dump_loop(&cb->args[1])) {
+	while (netlink_policy_dump_loop(cb->args[1])) {
 		void *hdr;
 		struct nlattr *nest;
 
@@ -1113,6 +1113,12 @@  static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+static int ctrl_dumppolicy_done(struct netlink_callback *cb)
+{
+	netlink_policy_dump_free(cb->args[1]);
+	return 0;
+}
+
 static const struct genl_ops genl_ctrl_ops[] = {
 	{
 		.cmd		= CTRL_CMD_GETFAMILY,
@@ -1123,6 +1129,7 @@  static const struct genl_ops genl_ctrl_ops[] = {
 	{
 		.cmd		= CTRL_CMD_GETPOLICY,
 		.dumpit		= ctrl_dumppolicy,
+		.done		= ctrl_dumppolicy_done,
 	},
 };
 
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index 641ffbdd977a..0176b59ce530 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -84,7 +84,6 @@  int netlink_policy_dump_start(const struct nla_policy *policy,
 	unsigned int policy_idx;
 	int err;
 
-	/* also returns 0 if "*_state" is our ERR_PTR() end marker */
 	if (*_state)
 		return 0;
 
@@ -140,21 +139,11 @@  static bool netlink_policy_dump_finished(struct nl_policy_dump *state)
 	       !state->policies[state->policy_idx].policy;
 }
 
-bool netlink_policy_dump_loop(unsigned long *_state)
+bool netlink_policy_dump_loop(unsigned long _state)
 {
-	struct nl_policy_dump *state = (void *)*_state;
-
-	if (IS_ERR(state))
-		return false;
-
-	if (netlink_policy_dump_finished(state)) {
-		kfree(state);
-		/* store end marker instead of freed state */
-		*_state = (unsigned long)ERR_PTR(-ENOENT);
-		return false;
-	}
+	struct nl_policy_dump *state = (void *)_state;
 
-	return true;
+	return !netlink_policy_dump_finished(state);
 }
 
 int netlink_policy_dump_write(struct sk_buff *skb, unsigned long _state)
@@ -309,3 +298,10 @@  int netlink_policy_dump_write(struct sk_buff *skb, unsigned long _state)
 	nla_nest_cancel(skb, policy);
 	return -ENOBUFS;
 }
+
+void netlink_policy_dump_free(unsigned long _state)
+{
+	struct nl_policy_dump *state = (void *)_state;
+
+	kfree(state);
+}