Hi Flavio,
On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote:
> Netfilter assumes that if the socket is present in the skb, then
> it can be used because that reference is cleaned up while the skb
> is crossing netns.
>
> We want to change that to preserve the socket reference in a future
> patch, so this is a preparation updating netfilter to check if the
> socket netns matches before use it.
>
> Signed-off-by: Flavio Leitner <[email protected]>
> Acked-by: Florian Westphal <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> ---
...
> --- a/net/netfilter/xt_socket.c
> +++ b/net/netfilter/xt_socket.c
> @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
> struct sk_buff *pskb = (struct sk_buff *)skb;
> struct sock *sk = skb->sk;
>
> + if (!net_eq(xt_net(par), sock_net(sk)))
> + sk = NULL;
> +
I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes
for me in read_pnet() because sk is NULL.
> if (!sk)
> sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par));
The old code seems to suggest that sk == NULL was possible.
I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I
can not guarantee that this really an upstream problem. The change seems
odd, though. Are you sure that it is not (or, rather, no longer) necessary
to check if sk == NULL before dereferencing it in sock_net() ?
> +
> if (sk) {
> bool wildcard;
> bool transparent = true;
> @@ -113,8 +117,12 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
> struct sk_buff *pskb = (struct sk_buff *)skb;
> struct sock *sk = skb->sk;
>
> + if (!net_eq(xt_net(par), sock_net(sk)))
> + sk = NULL;
> +
Same here.
> if (!sk)
> sk = nf_sk_lookup_slow_v6(xt_net(par), skb, xt_in(par));
> +
> if (sk) {
> bool wildcard;
> bool transparent = true;
Thanks,
Guenter
On Thu, Sep 27, 2018 at 01:46:29PM -0700, Guenter Roeck wrote:
> Hi Flavio,
>
> On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote:
> > Netfilter assumes that if the socket is present in the skb, then
> > it can be used because that reference is cleaned up while the skb
> > is crossing netns.
> >
> > We want to change that to preserve the socket reference in a future
> > patch, so this is a preparation updating netfilter to check if the
> > socket netns matches before use it.
> >
> > Signed-off-by: Flavio Leitner <[email protected]>
> > Acked-by: Florian Westphal <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> > ---
> ...
> > --- a/net/netfilter/xt_socket.c
> > +++ b/net/netfilter/xt_socket.c
> > @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
> > struct sk_buff *pskb = (struct sk_buff *)skb;
> > struct sock *sk = skb->sk;
> >
> > + if (!net_eq(xt_net(par), sock_net(sk)))
> > + sk = NULL;
> > +
>
> I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes
> for me in read_pnet() because sk is NULL.
>
> > if (!sk)
> > sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par));
>
> The old code seems to suggest that sk == NULL was possible.
>
> I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I
> can not guarantee that this really an upstream problem. The change seems
> odd, though. Are you sure that it is not (or, rather, no longer) necessary
> to check if sk == NULL before dereferencing it in sock_net() ?
Oops, it is necessary but if it's not and the netns doesn't match, we need
do the lookup. So, could you check if this fixes the problem for you?
From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001
From: Flavio Leitner <[email protected]>
Date: Thu, 27 Sep 2018 19:36:28 -0300
Subject: [PATCH] xt_socket: check sk before checking for netns.
Only check for the network namespace if the socket is available.
Fixes: f564650106a6 ("netfilter: check if the socket netns is correct.")
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Flavio Leitner <[email protected]>
---
net/netfilter/xt_socket.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
index 0472f3472842..ada144e5645b 100644
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -56,7 +56,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
struct sk_buff *pskb = (struct sk_buff *)skb;
struct sock *sk = skb->sk;
- if (!net_eq(xt_net(par), sock_net(sk)))
+ if (sk && !net_eq(xt_net(par), sock_net(sk)))
sk = NULL;
if (!sk)
@@ -117,7 +117,7 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
struct sk_buff *pskb = (struct sk_buff *)skb;
struct sock *sk = skb->sk;
- if (!net_eq(xt_net(par), sock_net(sk)))
+ if (sk && !net_eq(xt_net(par), sock_net(sk)))
sk = NULL;
if (!sk)
--
2.14.4
On Thu, Sep 27, 2018 at 07:58:24PM -0300, Flavio Leitner wrote:
> On Thu, Sep 27, 2018 at 01:46:29PM -0700, Guenter Roeck wrote:
> > Hi Flavio,
> >
> > On Wed, Jun 27, 2018 at 10:34:25AM -0300, Flavio Leitner wrote:
> > > Netfilter assumes that if the socket is present in the skb, then
> > > it can be used because that reference is cleaned up while the skb
> > > is crossing netns.
> > >
> > > We want to change that to preserve the socket reference in a future
> > > patch, so this is a preparation updating netfilter to check if the
> > > socket netns matches before use it.
> > >
> > > Signed-off-by: Flavio Leitner <[email protected]>
> > > Acked-by: Florian Westphal <[email protected]>
> > > Signed-off-by: David S. Miller <[email protected]>
> > > ---
> > ...
> > > --- a/net/netfilter/xt_socket.c
> > > +++ b/net/netfilter/xt_socket.c
> > > @@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
> > > struct sk_buff *pskb = (struct sk_buff *)skb;
> > > struct sock *sk = skb->sk;
> > >
> > > + if (!net_eq(xt_net(par), sock_net(sk)))
> > > + sk = NULL;
> > > +
> >
> > I am having trouble with this code. With CONFIG_NET_NS enabled, it crashes
> > for me in read_pnet() because sk is NULL.
> >
> > > if (!sk)
> > > sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par));
> >
> > The old code seems to suggest that sk == NULL was possible.
> >
> > I see the problem with the Chrome OS kernel rebased to v4.19-rc5, so I
> > can not guarantee that this really an upstream problem. The change seems
> > odd, though. Are you sure that it is not (or, rather, no longer) necessary
> > to check if sk == NULL before dereferencing it in sock_net() ?
>
> Oops, it is necessary but if it's not and the netns doesn't match, we need
> do the lookup. So, could you check if this fixes the problem for you?
>
> From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001
> From: Flavio Leitner <[email protected]>
> Date: Thu, 27 Sep 2018 19:36:28 -0300
> Subject: [PATCH] xt_socket: check sk before checking for netns.
>
> Only check for the network namespace if the socket is available.
>
> Fixes: f564650106a6 ("netfilter: check if the socket netns is correct.")
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Flavio Leitner <[email protected]>
This fixes the problem for me.
Tested-by: Guenter Roeck <[email protected]>
Thanks,
Guenter
> ---
> net/netfilter/xt_socket.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
> index 0472f3472842..ada144e5645b 100644
> --- a/net/netfilter/xt_socket.c
> +++ b/net/netfilter/xt_socket.c
> @@ -56,7 +56,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
> struct sk_buff *pskb = (struct sk_buff *)skb;
> struct sock *sk = skb->sk;
>
> - if (!net_eq(xt_net(par), sock_net(sk)))
> + if (sk && !net_eq(xt_net(par), sock_net(sk)))
> sk = NULL;
>
> if (!sk)
> @@ -117,7 +117,7 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
> struct sk_buff *pskb = (struct sk_buff *)skb;
> struct sock *sk = skb->sk;
>
> - if (!net_eq(xt_net(par), sock_net(sk)))
> + if (sk && !net_eq(xt_net(par), sock_net(sk)))
> sk = NULL;
>
> if (!sk)
> --
> 2.14.4
>
On Thu, Sep 27, 2018 at 07:58:24PM -0300, Flavio Leitner wrote:
[...]
> From a5f927e7f1368d753f87cb978d630d786d5adb62 Mon Sep 17 00:00:00 2001
> From: Flavio Leitner <[email protected]>
> Date: Thu, 27 Sep 2018 19:36:28 -0300
> Subject: [PATCH] xt_socket: check sk before checking for netns.
>
> Only check for the network namespace if the socket is available.
Applied.
Please, Cc [email protected] next time for your
netfilter patches.
Thanks.