* [PATCH] netfilter: ipv4: fix NULL dereference
@ 2016-03-23 14:27 Liping Zhang
2016-03-24 8:00 ` Nikolay Borisov
2016-03-24 20:22 ` Pablo Neira Ayuso
0 siblings, 2 replies; 8+ messages in thread
From: Liping Zhang @ 2016-03-23 14:27 UTC (permalink / raw)
To: netfilter-devel, pablo, kaber, davem; +Cc: kernel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
Commit fa50d974d104 ("ipv4: Namespaceify ip_default_ttl sysctl knob")
introduce the namespaceify ip_default_ttl, but sk_buff->sk maybe NULL,
so sock_net(skb->sk) will dereference the NULL pointer and oops will
happen.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/bridge/netfilter/nft_reject_bridge.c | 20 ++++++++++----------
net/ipv4/netfilter/ipt_SYNPROXY.c | 16 ++++++++++------
2 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
index adc8d72..77f7e7a 100644
--- a/net/bridge/netfilter/nft_reject_bridge.c
+++ b/net/bridge/netfilter/nft_reject_bridge.c
@@ -40,7 +40,8 @@ static void nft_reject_br_push_etherhdr(struct sk_buff *oldskb,
/* We cannot use oldskb->dev, it can be either bridge device (NF_BRIDGE INPUT)
* or the bridge port (NF_BRIDGE PREROUTING).
*/
-static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
+static void nft_reject_br_send_v4_tcp_reset(struct net *net,
+ struct sk_buff *oldskb,
const struct net_device *dev,
int hook)
{
@@ -48,7 +49,6 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
struct iphdr *niph;
const struct tcphdr *oth;
struct tcphdr _oth;
- struct net *net = sock_net(oldskb->sk);
if (!nft_bridge_iphdr_validate(oldskb))
return;
@@ -75,7 +75,8 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
br_deliver(br_port_get_rcu(dev), nskb);
}
-static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
+static void nft_reject_br_send_v4_unreach(struct net *net,
+ struct sk_buff *oldskb,
const struct net_device *dev,
int hook, u8 code)
{
@@ -86,7 +87,6 @@ static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
void *payload;
__wsum csum;
u8 proto;
- struct net *net = sock_net(oldskb->sk);
if (oldskb->csum_bad || !nft_bridge_iphdr_validate(oldskb))
return;
@@ -273,17 +273,17 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
case htons(ETH_P_IP):
switch (priv->type) {
case NFT_REJECT_ICMP_UNREACH:
- nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
- pkt->hook,
+ nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
+ pkt->in, pkt->hook,
priv->icmp_code);
break;
case NFT_REJECT_TCP_RST:
- nft_reject_br_send_v4_tcp_reset(pkt->skb, pkt->in,
- pkt->hook);
+ nft_reject_br_send_v4_tcp_reset(pkt->net, pkt->skb,
+ pkt->in, pkt->hook);
break;
case NFT_REJECT_ICMPX_UNREACH:
- nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
- pkt->hook,
+ nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
+ pkt->in, pkt->hook,
nft_reject_icmp_code(priv->icmp_code));
break;
}
diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
index 7b8fbb3..6b4f501 100644
--- a/net/ipv4/netfilter/ipt_SYNPROXY.c
+++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
@@ -18,10 +18,10 @@
#include <net/netfilter/nf_conntrack_synproxy.h>
static struct iphdr *
-synproxy_build_ip(struct sk_buff *skb, __be32 saddr, __be32 daddr)
+synproxy_build_ip(struct net *net, struct sk_buff *skb, __be32 saddr,
+ __be32 daddr)
{
struct iphdr *iph;
- struct net *net = sock_net(skb->sk);
skb_reset_network_header(skb);
iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
@@ -91,7 +91,8 @@ synproxy_send_client_synack(const struct synproxy_net *snet,
return;
skb_reserve(nskb, MAX_TCP_HEADER);
- niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
+ niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
+ iph->saddr);
skb_reset_transport_header(nskb);
nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
@@ -132,7 +133,8 @@ synproxy_send_server_syn(const struct synproxy_net *snet,
return;
skb_reserve(nskb, MAX_TCP_HEADER);
- niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
+ niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
+ iph->daddr);
skb_reset_transport_header(nskb);
nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
@@ -177,7 +179,8 @@ synproxy_send_server_ack(const struct synproxy_net *snet,
return;
skb_reserve(nskb, MAX_TCP_HEADER);
- niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
+ niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
+ iph->saddr);
skb_reset_transport_header(nskb);
nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
@@ -215,7 +218,8 @@ synproxy_send_client_ack(const struct synproxy_net *snet,
return;
skb_reserve(nskb, MAX_TCP_HEADER);
- niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
+ niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
+ iph->daddr);
skb_reset_transport_header(nskb);
nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-23 14:27 [PATCH] netfilter: ipv4: fix NULL dereference Liping Zhang
@ 2016-03-24 8:00 ` Nikolay Borisov
[not found] ` <54382a12.8a92.153a7ba19ff.Coremail.zlpwmdx@163.com>
2016-03-24 20:16 ` Pablo Neira Ayuso
2016-03-24 20:22 ` Pablo Neira Ayuso
1 sibling, 2 replies; 8+ messages in thread
From: Nikolay Borisov @ 2016-03-24 8:00 UTC (permalink / raw)
To: Liping Zhang, netfilter-devel, pablo, kaber, davem; +Cc: Liping Zhang
I've been running production kernels in production with those changes
and so far I haven't observed a single crash resulting from this.
Furthermore, I believe that all the call sites of synproxy_build_ip
should have the skb associated with a valid tcp socket, which must have
originated from a particular namespace.
On 03/23/2016 04:27 PM, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> Commit fa50d974d104 ("ipv4: Namespaceify ip_default_ttl sysctl knob")
> introduce the namespaceify ip_default_ttl, but sk_buff->sk maybe NULL,
> so sock_net(skb->sk) will dereference the NULL pointer and oops will
> happen.
>
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
> ---
> net/bridge/netfilter/nft_reject_bridge.c | 20 ++++++++++----------
> net/ipv4/netfilter/ipt_SYNPROXY.c | 16 ++++++++++------
> 2 files changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
> index adc8d72..77f7e7a 100644
> --- a/net/bridge/netfilter/nft_reject_bridge.c
> +++ b/net/bridge/netfilter/nft_reject_bridge.c
> @@ -40,7 +40,8 @@ static void nft_reject_br_push_etherhdr(struct sk_buff *oldskb,
> /* We cannot use oldskb->dev, it can be either bridge device (NF_BRIDGE INPUT)
> * or the bridge port (NF_BRIDGE PREROUTING).
> */
> -static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
> +static void nft_reject_br_send_v4_tcp_reset(struct net *net,
> + struct sk_buff *oldskb,
> const struct net_device *dev,
> int hook)
> {
> @@ -48,7 +49,6 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
> struct iphdr *niph;
> const struct tcphdr *oth;
> struct tcphdr _oth;
> - struct net *net = sock_net(oldskb->sk);
>
> if (!nft_bridge_iphdr_validate(oldskb))
> return;
> @@ -75,7 +75,8 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
> br_deliver(br_port_get_rcu(dev), nskb);
> }
>
> -static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
> +static void nft_reject_br_send_v4_unreach(struct net *net,
> + struct sk_buff *oldskb,
> const struct net_device *dev,
> int hook, u8 code)
> {
> @@ -86,7 +87,6 @@ static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
> void *payload;
> __wsum csum;
> u8 proto;
> - struct net *net = sock_net(oldskb->sk);
>
> if (oldskb->csum_bad || !nft_bridge_iphdr_validate(oldskb))
> return;
> @@ -273,17 +273,17 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
> case htons(ETH_P_IP):
> switch (priv->type) {
> case NFT_REJECT_ICMP_UNREACH:
> - nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
> - pkt->hook,
> + nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
> + pkt->in, pkt->hook,
> priv->icmp_code);
> break;
> case NFT_REJECT_TCP_RST:
> - nft_reject_br_send_v4_tcp_reset(pkt->skb, pkt->in,
> - pkt->hook);
> + nft_reject_br_send_v4_tcp_reset(pkt->net, pkt->skb,
> + pkt->in, pkt->hook);
> break;
> case NFT_REJECT_ICMPX_UNREACH:
> - nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
> - pkt->hook,
> + nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
> + pkt->in, pkt->hook,
> nft_reject_icmp_code(priv->icmp_code));
> break;
> }
> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
> index 7b8fbb3..6b4f501 100644
> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c
> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
> @@ -18,10 +18,10 @@
> #include <net/netfilter/nf_conntrack_synproxy.h>
>
> static struct iphdr *
> -synproxy_build_ip(struct sk_buff *skb, __be32 saddr, __be32 daddr)
> +synproxy_build_ip(struct net *net, struct sk_buff *skb, __be32 saddr,
> + __be32 daddr)
> {
> struct iphdr *iph;
> - struct net *net = sock_net(skb->sk);
>
> skb_reset_network_header(skb);
> iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
> @@ -91,7 +91,8 @@ synproxy_send_client_synack(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
> + iph->saddr);
>
> skb_reset_transport_header(nskb);
> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> @@ -132,7 +133,8 @@ synproxy_send_server_syn(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
> + iph->daddr);
>
> skb_reset_transport_header(nskb);
> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> @@ -177,7 +179,8 @@ synproxy_send_server_ack(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
> + iph->saddr);
>
> skb_reset_transport_header(nskb);
> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> @@ -215,7 +218,8 @@ synproxy_send_client_ack(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
> + iph->daddr);
>
> skb_reset_transport_header(nskb);
> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: ipv4: fix NULL dereference
[not found] ` <54382a12.8a92.153a7ba19ff.Coremail.zlpwmdx@163.com>
@ 2016-03-24 8:45 ` Nikolay Borisov
0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2016-03-24 8:45 UTC (permalink / raw)
To: 张李平
Cc: netfilter-devel, pablo, kaber, davem, Liping Zhang
On 03/24/2016 10:25 AM, 张李平 wrote:
> At 2016-03-24 16:00:02, "Nikolay Borisov" <kernel@kyup.com> wrote:
>> I've been running production kernels in production with those changes
>> and so far I haven't observed a single crash resulting from this.
>
> Did you run the test with the CONFIG_NET_NS config enabled?
Of course, why else would I author the patches :)
>
>
>> Furthermore, I believe that all the call sites of synproxy_build_ip
>> should have the skb associated with a valid tcp socket, which must have
>
>> originated from a particular namespace.
>
>
> Actually, the sk_buff passed to synproxy_build_ip() are all alloced by
> alloc_skb, and was not associated with a specific tcp socket, so skb->sk
> is always NULL. Further more, SYNPROXY target can be used to proxy
> the tcp connections which are not local, i.e. tcp sockets was located on
> other hosts.
Admittedly I'm not using the synproxy module so that might explain why I
haven't seen crashes.
In any case you can add my:
Reviewed-by: Nikolay Borisov <kernel@kyup.com>
>
>
>>
>> On 03/23/2016 04:27 PM, Liping Zhang wrote:
>>> From: Liping Zhang <liping.zhang@spreadtrum.com>
>>>
>>> Commit fa50d974d104 ("ipv4: Namespaceify ip_default_ttl sysctl knob")
>>> introduce the namespaceify ip_default_ttl, but sk_buff->sk maybe NULL,
>>> so sock_net(skb->sk) will dereference the NULL pointer and oops will
>>> happen.
>>>
>>> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
>>> ---
>>> net/bridge/netfilter/nft_reject_bridge.c | 20 ++++++++++----------
>>> net/ipv4/netfilter/ipt_SYNPROXY.c | 16 ++++++++++------
>>> 2 files changed, 20 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
>>> index adc8d72..77f7e7a 100644
>>> --- a/net/bridge/netfilter/nft_reject_bridge.c
>>> +++ b/net/bridge/netfilter/nft_reject_bridge.c
>>> @@ -40,7 +40,8 @@ static void nft_reject_br_push_etherhdr(struct sk_buff *oldskb,
>>> /* We cannot use oldskb->dev, it can be either bridge device (NF_BRIDGE INPUT)
>>> * or the bridge port (NF_BRIDGE PREROUTING).
>>> */
>>> -static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
>>> +static void nft_reject_br_send_v4_tcp_reset(struct net *net,
>>> + struct sk_buff *oldskb,
>>> const struct net_device *dev,
>>> int hook)
>>> {
>>> @@ -48,7 +49,6 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
>>> struct iphdr *niph;
>>> const struct tcphdr *oth;
>>> struct tcphdr _oth;
>>> - struct net *net = sock_net(oldskb->sk);
>>>
>>> if (!nft_bridge_iphdr_validate(oldskb))
>>> return;
>>> @@ -75,7 +75,8 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
>>> br_deliver(br_port_get_rcu(dev), nskb);
>>> }
>>>
>>> -static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
>>> +static void nft_reject_br_send_v4_unreach(struct net *net,
>>> + struct sk_buff *oldskb,
>>> const struct net_device *dev,
>>> int hook, u8 code)
>>> {
>>> @@ -86,7 +87,6 @@ static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
>>> void *payload;
>>> __wsum csum;
>>> u8 proto;
>>> - struct net *net = sock_net(oldskb->sk);
>>>
>>> if (oldskb->csum_bad || !nft_bridge_iphdr_validate(oldskb))
>>> return;
>>> @@ -273,17 +273,17 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
>>> case htons(ETH_P_IP):
>>> switch (priv->type) {
>>> case NFT_REJECT_ICMP_UNREACH:
>>> - nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
>>> - pkt->hook,
>>> + nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
>>> + pkt->in, pkt->hook,
>>> priv->icmp_code);
>>> break;
>>> case NFT_REJECT_TCP_RST:
>>> - nft_reject_br_send_v4_tcp_reset(pkt->skb, pkt->in,
>>> - pkt->hook);
>>> + nft_reject_br_send_v4_tcp_reset(pkt->net, pkt->skb,
>>> + pkt->in, pkt->hook);
>>> break;
>>> case NFT_REJECT_ICMPX_UNREACH:
>>> - nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
>>> - pkt->hook,
>>> + nft_reject_br_send_v4_unreach(pkt->net, pkt->skb,
>>> + pkt->in, pkt->hook,
>>> nft_reject_icmp_code(priv->icmp_code));
>>> break;
>>> }
>>> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
>>> index 7b8fbb3..6b4f501 100644
>>> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c
>>> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
>>> @@ -18,10 +18,10 @@
>>> #include <net/netfilter/nf_conntrack_synproxy.h>
>>>
>>> static struct iphdr *
>>> -synproxy_build_ip(struct sk_buff *skb, __be32 saddr, __be32 daddr)
>>> +synproxy_build_ip(struct net *net, struct sk_buff *skb, __be32 saddr,
>>> + __be32 daddr)
>>> {
>>> struct iphdr *iph;
>>> - struct net *net = sock_net(skb->sk);
>>>
>>> skb_reset_network_header(skb);
>>> iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
>>> @@ -91,7 +91,8 @@ synproxy_send_client_synack(const struct synproxy_net *snet,
>>> return;
>>> skb_reserve(nskb, MAX_TCP_HEADER);
>>>
>>> - niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
>>> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
>>> + iph->saddr);
>>>
>>> skb_reset_transport_header(nskb);
>>> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
>>> @@ -132,7 +133,8 @@ synproxy_send_server_syn(const struct synproxy_net *snet,
>>> return;
>>> skb_reserve(nskb, MAX_TCP_HEADER);
>>>
>>> - niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
>>> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
>>> + iph->daddr);
>>>
>>> skb_reset_transport_header(nskb);
>>> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
>>> @@ -177,7 +179,8 @@ synproxy_send_server_ack(const struct synproxy_net *snet,
>>> return;
>>> skb_reserve(nskb, MAX_TCP_HEADER);
>>>
>>> - niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
>>> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
>>> + iph->saddr);
>>>
>>> skb_reset_transport_header(nskb);
>>> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
>>> @@ -215,7 +218,8 @@ synproxy_send_client_ack(const struct synproxy_net *snet,
>>> return;
>>> skb_reserve(nskb, MAX_TCP_HEADER);
>>>
>>> - niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
>>> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
>>> + iph->daddr);
>>>
>>> skb_reset_transport_header(nskb);
>>> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
>>>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-24 8:00 ` Nikolay Borisov
[not found] ` <54382a12.8a92.153a7ba19ff.Coremail.zlpwmdx@163.com>
@ 2016-03-24 20:16 ` Pablo Neira Ayuso
1 sibling, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-24 20:16 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Liping Zhang, netfilter-devel, kaber, davem, Liping Zhang
On Thu, Mar 24, 2016 at 10:00:02AM +0200, Nikolay Borisov wrote:
> I've been running production kernels in production with those changes
> and so far I haven't observed a single crash resulting from this.
> Furthermore, I believe that all the call sites of synproxy_build_ip
> should have the skb associated with a valid tcp socket, which must have
> originated from a particular namespace.
Please, always Cc: netfilter-devel@vger.kernel.org for patches that
modify netfilter code. Your change is buggy, we cannot assume skb->sk
set on packets that are being forwarded, we could have detected this
following this process.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-23 14:27 [PATCH] netfilter: ipv4: fix NULL dereference Liping Zhang
2016-03-24 8:00 ` Nikolay Borisov
@ 2016-03-24 20:22 ` Pablo Neira Ayuso
2016-03-25 6:38 ` Liping Zhang
2016-03-25 7:24 ` Liping Zhang
1 sibling, 2 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-24 20:22 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, kaber, davem, kernel, Liping Zhang
On Wed, Mar 23, 2016 at 10:27:30PM +0800, Liping Zhang wrote:
> diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c b/net/ipv4/netfilter/ipt_SYNPROXY.c
> index 7b8fbb3..6b4f501 100644
> --- a/net/ipv4/netfilter/ipt_SYNPROXY.c
> +++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
> @@ -18,10 +18,10 @@
> #include <net/netfilter/nf_conntrack_synproxy.h>
>
> static struct iphdr *
> -synproxy_build_ip(struct sk_buff *skb, __be32 saddr, __be32 daddr)
> +synproxy_build_ip(struct net *net, struct sk_buff *skb, __be32 saddr,
> + __be32 daddr)
> {
> struct iphdr *iph;
> - struct net *net = sock_net(skb->sk);
>
> skb_reset_network_header(skb);
> iph = (struct iphdr *)skb_put(skb, sizeof(*iph));
> @@ -91,7 +91,8 @@ synproxy_send_client_synack(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->daddr, iph->saddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->daddr,
> + iph->saddr);
>
> skb_reset_transport_header(nskb);
> nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> @@ -132,7 +133,8 @@ synproxy_send_server_syn(const struct synproxy_net *snet,
> return;
> skb_reserve(nskb, MAX_TCP_HEADER);
>
> - niph = synproxy_build_ip(nskb, iph->saddr, iph->daddr);
> + niph = synproxy_build_ip(nf_ct_net(snet->tmpl), nskb, iph->saddr,
> + iph->daddr);
Could you also pass net as parameter to synproxy_send_server_syn() ?
par->net provides this from synproxy_tg4().
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re:Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-24 20:22 ` Pablo Neira Ayuso
@ 2016-03-25 6:38 ` Liping Zhang
2016-03-25 10:49 ` Pablo Neira Ayuso
2016-03-25 7:24 ` Liping Zhang
1 sibling, 1 reply; 8+ messages in thread
From: Liping Zhang @ 2016-03-25 6:38 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, kaber, davem, kernel, Liping Zhang
At 2016-03-25 04:22:05, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>
>Could you also pass net as parameter to synproxy_send_server_syn() ?
>
>par->net provides this from synproxy_tg4().
Not pass the net but replace the first parameter 'snet' with 'net' seems better?
snet is only used in synproxy_recv_client_ack->synproxy_send_server_syn call path,
and in other call path, actually we only need net, and we can call synproxy_pernet(net)
to get the snet.
like follows:
-synproxy_send_server_syn(const struct synproxy_net *snet,
+synproxy_send_server_syn(struct net *net,
-synproxy_recv_client_ack(const struct synproxy_net *snet,
+synproxy_recv_client_ack(struct net *net,
const struct sk_buff *skb, const struct tcphdr *th,
struct synproxy_options *opts, u32 recv_seq)
{
+ struct synproxy_net *snet = synproxy_pernet(net);
>
>Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-24 20:22 ` Pablo Neira Ayuso
2016-03-25 6:38 ` Liping Zhang
@ 2016-03-25 7:24 ` Liping Zhang
1 sibling, 0 replies; 8+ messages in thread
From: Liping Zhang @ 2016-03-25 7:24 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, kaber, davem, kernel, Liping Zhang
At 2016-03-25 04:22:05, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>
>Could you also pass net as parameter to synproxy_send_server_syn() ?
>
>par->net provides this from synproxy_tg4().
Not pass the net but replace the first parameter 'snet' with 'net' seems better?
snet is only used in synproxy_recv_client_ack->synproxy_send_server_syn call path,
and in other call path, actually we only need net, and we can call synproxy_pernet(net)
to get the snet.
like follows:
-synproxy_send_server_syn(const struct synproxy_net *snet,
+synproxy_send_server_syn(struct net *net,
-synproxy_recv_client_ack(const struct synproxy_net *snet,
+synproxy_recv_client_ack(struct net *net,
const struct sk_buff *skb, const struct tcphdr *th,
struct synproxy_options *opts, u32 recv_seq) {
+ struct synproxy_net *snet = synproxy_pernet(net);
>
>Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Re: [PATCH] netfilter: ipv4: fix NULL dereference
2016-03-25 6:38 ` Liping Zhang
@ 2016-03-25 10:49 ` Pablo Neira Ayuso
0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-25 10:49 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, kaber, davem, kernel, Liping Zhang
On Fri, Mar 25, 2016 at 02:38:15PM +0800, Liping Zhang wrote:
> At 2016-03-25 04:22:05, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> >
> >Could you also pass net as parameter to synproxy_send_server_syn() ?
> >
> >par->net provides this from synproxy_tg4().
>
> Not pass the net but replace the first parameter 'snet' with 'net' seems better?
> snet is only used in synproxy_recv_client_ack->synproxy_send_server_syn call path,
> and in other call path, actually we only need net, and we can call synproxy_pernet(net)
> to get the snet.
>
> like follows:
> -synproxy_send_server_syn(const struct synproxy_net *snet,
> +synproxy_send_server_syn(struct net *net,
>
> -synproxy_recv_client_ack(const struct synproxy_net *snet,
> +synproxy_recv_client_ack(struct net *net,
> const struct sk_buff *skb, const struct tcphdr *th,
> struct synproxy_options *opts, u32 recv_seq)
> {
> + struct synproxy_net *snet = synproxy_pernet(net);
Fine with me.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-03-25 10:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-23 14:27 [PATCH] netfilter: ipv4: fix NULL dereference Liping Zhang
2016-03-24 8:00 ` Nikolay Borisov
[not found] ` <54382a12.8a92.153a7ba19ff.Coremail.zlpwmdx@163.com>
2016-03-24 8:45 ` Nikolay Borisov
2016-03-24 20:16 ` Pablo Neira Ayuso
2016-03-24 20:22 ` Pablo Neira Ayuso
2016-03-25 6:38 ` Liping Zhang
2016-03-25 10:49 ` Pablo Neira Ayuso
2016-03-25 7:24 ` Liping Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).