Message ID | 3a97c044d7e99dc4bd3d6b8d36d79a01@visp.net.lb |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
On Sun, May 04, 2014 at 10:25:58AM +0300, Denys Fedoryshchenko wrote: > Hi > > I bit more debugging and found that problem is happening at: > > >sock = netlink_lookup(sock_net(ssk), ssk->sk_protocol, portid); > > ssk is NULL > > After checking, i noticed in nfnetlink.c > nfnetlink_rcv_batch() function > > We have > nskb->sk = oskb->sk; > skb = nskb; > > I am matching condition > ss = rcu_dereference_protected(table[subsys_id].subsys, > lockdep_is_held(&table[subsys_id].mutex)); > if (!ss) { > > And then > nfnl_unlock(subsys_id); > kfree_skb(nskb); > return netlink_ack(skb, nlh, -EOPNOTSUPP); > > If i am not wrong, nskb same pointer as skb, so we are giving > netlink_ack freed pointer? > Is it "use after free()" ? Right, this is an embarrasing use after free when no nf_tables support has been selected / modules are not available. > If yes, then it seems attached patch fixing my issue. Please let me > know, if it is ok and i should submit it. I'm going to take this, but please next time use git format-patch and include your Signed-off-by tag. If you feel the patch is not complete in some aspect or that you may be missing anything, just include the RFC tag in the subject. Thanks Denys! -- 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 Thanks, understood about RFC and Signed-off-by. Next time will do that. I was not sure i have correct idea what is happening in code, so maybe patch was totally incorrect. On 2014-05-04 14:33, Pablo Neira Ayuso wrote: > On Sun, May 04, 2014 at 10:25:58AM +0300, Denys Fedoryshchenko wrote: >> Hi >> >> I bit more debugging and found that problem is happening at: >> >> >sock = netlink_lookup(sock_net(ssk), ssk->sk_protocol, portid); >> >> ssk is NULL >> >> After checking, i noticed in nfnetlink.c >> nfnetlink_rcv_batch() function >> >> We have >> nskb->sk = oskb->sk; >> skb = nskb; >> >> I am matching condition >> ss = rcu_dereference_protected(table[subsys_id].subsys, >> lockdep_is_held(&table[subsys_id].mutex)); >> if (!ss) { >> >> And then >> nfnl_unlock(subsys_id); >> kfree_skb(nskb); >> return netlink_ack(skb, nlh, -EOPNOTSUPP); >> >> If i am not wrong, nskb same pointer as skb, so we are giving >> netlink_ack freed pointer? >> Is it "use after free()" ? > > Right, this is an embarrasing use after free when no nf_tables support > has been selected / modules are not available. > >> If yes, then it seems attached patch fixing my issue. Please let me >> know, if it is ok and i should submit it. > > I'm going to take this, but please next time use git format-patch and > include your Signed-off-by tag. If you feel the patch is not complete > in some aspect or that you may be missing anything, just include the > RFC tag in the subject. > > Thanks Denys! -- 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
--- linux-3.14.2/net/netfilter/nfnetlink.c 2014-04-27 03:19:26.000000000 +0300 +++ linux-3.14.2-modified/net/netfilter/nfnetlink.c 2014-05-04 10:14:32.108299766 +0300 @@ -248,15 +248,15 @@ replay: #endif { nfnl_unlock(subsys_id); - kfree_skb(nskb); - return netlink_ack(skb, nlh, -EOPNOTSUPP); + netlink_ack(skb, nlh, -EOPNOTSUPP); + return kfree_skb(nskb); } } if (!ss->commit || !ss->abort) { nfnl_unlock(subsys_id); - kfree_skb(nskb); - return netlink_ack(skb, nlh, -EOPNOTSUPP); + netlink_ack(skb, nlh, -EOPNOTSUPP); + return kfree_skb(skb); } while (skb->len >= nlmsg_total_size(0)) {