[PATCH] TIPC: Fix infinite loop in netlink handler

[PATCH] TIPC: Fix infinite loop in netlink handler

[Florian Westphal]

From: Florian Westphal <fw@strlen.de>

The tipc netlink config handler uses the nlmsg_pid from the
request header as destination for its reply. If the application
initialized nlmsg_pid to 0, the reply is looped back to the kernel,
causing hangup. Fix: use nlmsg_pid of the skb that triggered the
request.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/tipc/netlink.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c
index 4cdafa2..6a7f7b4 100644
--- a/net/tipc/netlink.c
+++ b/net/tipc/netlink.c
@@ -60,7 +60,7 @@ static int handle_cmd(struct sk_buff *skb, struct genl_info *info)
 		rep_nlh = nlmsg_hdr(rep_buf);
 		memcpy(rep_nlh, req_nlh, hdr_space);
 		rep_nlh->nlmsg_len = rep_buf->len;
-		genlmsg_unicast(rep_buf, req_nlh->nlmsg_pid);
+		genlmsg_unicast(rep_buf, NETLINK_CB(skb).pid);
 	}
 
 	return 0;

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TIPC: Fix infinite loop in netlink handler

[Patrick McHardy]

Florian Westphal wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> The tipc netlink config handler uses the nlmsg_pid from the
> request header as destination for its reply. If the application
> initialized nlmsg_pid to 0, the reply is looped back to the kernel,
> causing hangup. Fix: use nlmsg_pid of the skb that triggered the
> request.
> 
> -		genlmsg_unicast(rep_buf, req_nlh->nlmsg_pid);
> +		genlmsg_unicast(rep_buf, NETLINK_CB(skb).pid);


This is the second time we're seeing this bug within a few weeks,
maybe we should rename NETLINK_CB(skb).pid to dst_pid to avoid
similar confusion in the future? We could even rename nlmsg_pid
to nlmsg_src within the kernel, which should make it completely
clear what is being refered to.

Re: [PATCH] TIPC: Fix infinite loop in netlink handler

[Stephens, Allan]

Patrick McHardy wrote:
 
> Florian Westphal wrote:
> > From: Florian Westphal <fw@strlen.de>
> > 
> > The tipc netlink config handler uses the nlmsg_pid from the request 
> > header as destination for its reply. If the application initialized 
> > nlmsg_pid to 0, the reply is looped back to the kernel, causing 
> > hangup. Fix: use nlmsg_pid of the skb that triggered the request.
> > 
> > -		genlmsg_unicast(rep_buf, req_nlh->nlmsg_pid);
> > +		genlmsg_unicast(rep_buf, NETLINK_CB(skb).pid);
> 
> 
> This is the second time we're seeing this bug within a few 
> weeks, maybe we should rename NETLINK_CB(skb).pid to dst_pid 
> to avoid similar confusion in the future? We could even 
> rename nlmsg_pid to nlmsg_src within the kernel, which should 
> make it completely clear what is being refered to.

I'm assuming that Patrick's first suggestion should have read "rename
NETLINK_CB(skb).pid to src_pid", as there is already a "dst_pid" field
in the netlink_skb_parms structure type.

I agree that such a change would make things clearer to folks who are
relative novices when it comes to Netlink.

-- Al

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TIPC: Fix infinite loop in netlink handler

[Florian Westphal]

Stephens, Allan <allan.stephens@windriver.com> wrote:
[removed tipc-discussion list from CC]
> Patrick McHardy wrote:
> > Florian Westphal wrote:
> > > -		genlmsg_unicast(rep_buf, req_nlh->nlmsg_pid);
> > > +		genlmsg_unicast(rep_buf, NETLINK_CB(skb).pid);
> > 
> > 
> > This is the second time we're seeing this bug within a few 
> > weeks, maybe we should rename NETLINK_CB(skb).pid to dst_pid 
> > to avoid similar confusion in the future? We could even 
> > rename nlmsg_pid to nlmsg_src within the kernel, which should 
> > make it completely clear what is being refered to.
> 
> I'm assuming that Patrick's first suggestion should have read "rename
> NETLINK_CB(skb).pid to src_pid", as there is already a "dst_pid" field
> in the netlink_skb_parms structure type.

Hmm, this dst_pid got removed in commit
4e9b82693542003b028c8494e9e3c49615b91ce7.

And im actually not sure if a rename would really help things.
The problem was/is the usage of the original header value (which works just
fine if userspace plays along). So the only real solution
would be to update the nlmsg_pid value to the NETLINK_CB(skb).pid if
it is 0 before the 'doit' callback in genl_rcv_msg() -- and this seems
just the wrong thing to to. Plus, there is a snd_seq field in the info
structure passed to the callback which always has the NETLINK_CB(skb).pid.


Perhaps something like the following would be a start?

./include/linux/netlink.h:
-         __u32           nlmsg_pid;      /* Sending process port ID */
+         __u32           nlmsg_pid;      /* Sending process port ID -
+  must not be used as destination parameter with
+  genlmsg_unicast() and friends as it might be 0 meaning 'send to kernel' */

Thoughts?