Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Paul Moore]

Hello,

I'm currently trying to debug a problem with 4.7-rc1 and labeled
networking over UDP.  I'm having some difficulty with the latest
4.7-rc1 builds on my test system at the moment so I haven't been able
to concisely identify the problem, but looking through the commits in
4.7-rc1 I think there may be a problem with the following:

  commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
  Author: samanthakumar <samanthakumar@google.com>
  Date:   Tue Apr 5 12:41:15 2016 -0400

   udp: remove headers from UDP packets before queueing

   Remove UDP transport headers before queueing packets for reception.
   This change simplifies a follow-up patch to add MSG_PEEK support.

   Signed-off-by: Sam Kumar <samanthakumar@google.com>
   Signed-off-by: Willem de Bruijn <willemb@google.com>
   Signed-off-by: David S. Miller <davem@davemloft.net>

... it appears that this commit changes things so that sk_filter() is
only called when sk->sk_filter is not NULL.  While this is fine for
the traditional socket filter case, it causes problems with LSMs that
make use of security_sock_rcv_skb() to enforce per-packet access
controls.

Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
bisection test around this patch, but I wanted to mention this now in
case others are seeing the same problem.

-- 
paul moore
www.paul-moore.com

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Eric Dumazet]

On Wed, 2016-06-01 at 15:01 -0400, Paul Moore wrote:
> Hello,
> 
> I'm currently trying to debug a problem with 4.7-rc1 and labeled
> networking over UDP.  I'm having some difficulty with the latest
> 4.7-rc1 builds on my test system at the moment so I haven't been able
> to concisely identify the problem, but looking through the commits in
> 4.7-rc1 I think there may be a problem with the following:
> 
>   commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
>   Author: samanthakumar <samanthakumar@google.com>
>   Date:   Tue Apr 5 12:41:15 2016 -0400
> 
>    udp: remove headers from UDP packets before queueing
> 
>    Remove UDP transport headers before queueing packets for reception.
>    This change simplifies a follow-up patch to add MSG_PEEK support.
> 
>    Signed-off-by: Sam Kumar <samanthakumar@google.com>
>    Signed-off-by: Willem de Bruijn <willemb@google.com>
>    Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> ... it appears that this commit changes things so that sk_filter() is
> only called when sk->sk_filter is not NULL.  While this is fine for
> the traditional socket filter case, it causes problems with LSMs that
> make use of security_sock_rcv_skb() to enforce per-packet access
> controls.
> 
> Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
> bisection test around this patch, but I wanted to mention this now in
> case others are seeing the same problem.
> 

Thanks for the report. Please try following fix.

sk_filter() got additional features like the skb_pfmemalloc() things and
security_sock_rcv_skb()

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d56c0559b477..0ff31d97d485 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1618,12 +1618,12 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		}
 	}
 
-	if (rcu_access_pointer(sk->sk_filter)) {
-		if (udp_lib_checksum_complete(skb))
+	if (rcu_access_pointer(sk->sk_filter) &&
+	    udp_lib_checksum_complete(skb))
 			goto csum_error;
-		if (sk_filter(sk, skb))
-			goto drop;
-	}
+
+	if (sk_filter(sk, skb))
+		goto drop;
 
 	udp_csum_pull_header(skb);
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 2da1896af934..f421c9f23c5b 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -653,12 +653,12 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		}
 	}
 
-	if (rcu_access_pointer(sk->sk_filter)) {
-		if (udp_lib_checksum_complete(skb))
-			goto csum_error;
-		if (sk_filter(sk, skb))
-			goto drop;
-	}
+	if (rcu_access_pointer(sk->sk_filter) &&
+	    udp_lib_checksum_complete(skb))
+		goto csum_error;
+
+	if (sk_filter(sk, skb))
+		goto drop;
 
 	udp_csum_pull_header(skb);
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Stephen Smalley]

On 06/01/2016 03:18 PM, Eric Dumazet wrote:
> On Wed, 2016-06-01 at 15:01 -0400, Paul Moore wrote:
>> Hello,
>>
>> I'm currently trying to debug a problem with 4.7-rc1 and labeled
>> networking over UDP.  I'm having some difficulty with the latest
>> 4.7-rc1 builds on my test system at the moment so I haven't been able
>> to concisely identify the problem, but looking through the commits in
>> 4.7-rc1 I think there may be a problem with the following:
>>
>>   commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
>>   Author: samanthakumar <samanthakumar-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>   Date:   Tue Apr 5 12:41:15 2016 -0400
>>
>>    udp: remove headers from UDP packets before queueing
>>
>>    Remove UDP transport headers before queueing packets for reception.
>>    This change simplifies a follow-up patch to add MSG_PEEK support.
>>
>>    Signed-off-by: Sam Kumar <samanthakumar-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>    Signed-off-by: Willem de Bruijn <willemb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>>    Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
>>
>> ... it appears that this commit changes things so that sk_filter() is
>> only called when sk->sk_filter is not NULL.  While this is fine for
>> the traditional socket filter case, it causes problems with LSMs that
>> make use of security_sock_rcv_skb() to enforce per-packet access
>> controls.
>>
>> Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
>> bisection test around this patch, but I wanted to mention this now in
>> case others are seeing the same problem.
>>
> 
> Thanks for the report. Please try following fix.
> 
> sk_filter() got additional features like the skb_pfmemalloc() things and
> security_sock_rcv_skb()

This resolved the SELinux regression for me.

Tested-by: Stephen Smalley <sds-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>

> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index d56c0559b477..0ff31d97d485 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1618,12 +1618,12 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  		}
>  	}
>  
> -	if (rcu_access_pointer(sk->sk_filter)) {
> -		if (udp_lib_checksum_complete(skb))
> +	if (rcu_access_pointer(sk->sk_filter) &&
> +	    udp_lib_checksum_complete(skb))
>  			goto csum_error;
> -		if (sk_filter(sk, skb))
> -			goto drop;
> -	}
> +
> +	if (sk_filter(sk, skb))
> +		goto drop;
>  
>  	udp_csum_pull_header(skb);
>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 2da1896af934..f421c9f23c5b 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -653,12 +653,12 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  		}
>  	}
>  
> -	if (rcu_access_pointer(sk->sk_filter)) {
> -		if (udp_lib_checksum_complete(skb))
> -			goto csum_error;
> -		if (sk_filter(sk, skb))
> -			goto drop;
> -	}
> +	if (rcu_access_pointer(sk->sk_filter) &&
> +	    udp_lib_checksum_complete(skb))
> +		goto csum_error;
> +
> +	if (sk_filter(sk, skb))
> +		goto drop;
>  
>  	udp_csum_pull_header(skb);
>  	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> 
> 
> _______________________________________________
> Selinux mailing list
> Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
> To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
> To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
> 

_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Paul Moore]

On Wed, Jun 1, 2016 at 4:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 06/01/2016 03:18 PM, Eric Dumazet wrote:
>> On Wed, 2016-06-01 at 15:01 -0400, Paul Moore wrote:
>>> Hello,
>>>
>>> I'm currently trying to debug a problem with 4.7-rc1 and labeled
>>> networking over UDP.  I'm having some difficulty with the latest
>>> 4.7-rc1 builds on my test system at the moment so I haven't been able
>>> to concisely identify the problem, but looking through the commits in
>>> 4.7-rc1 I think there may be a problem with the following:
>>>
>>>   commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
>>>   Author: samanthakumar <samanthakumar@google.com>
>>>   Date:   Tue Apr 5 12:41:15 2016 -0400
>>>
>>>    udp: remove headers from UDP packets before queueing
>>>
>>>    Remove UDP transport headers before queueing packets for reception.
>>>    This change simplifies a follow-up patch to add MSG_PEEK support.
>>>
>>>    Signed-off-by: Sam Kumar <samanthakumar@google.com>
>>>    Signed-off-by: Willem de Bruijn <willemb@google.com>
>>>    Signed-off-by: David S. Miller <davem@davemloft.net>
>>>
>>> ... it appears that this commit changes things so that sk_filter() is
>>> only called when sk->sk_filter is not NULL.  While this is fine for
>>> the traditional socket filter case, it causes problems with LSMs that
>>> make use of security_sock_rcv_skb() to enforce per-packet access
>>> controls.
>>>
>>> Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
>>> bisection test around this patch, but I wanted to mention this now in
>>> case others are seeing the same problem.
>>
>> Thanks for the report. Please try following fix.
>>
>> sk_filter() got additional features like the skb_pfmemalloc() things and
>> security_sock_rcv_skb()
>
> This resolved the SELinux regression for me.
>
> Tested-by: Stephen Smalley <sds@tycho.nsa.gov>

The patch works for me too.  Eric, are you going to send this to DaveM
(assuming he isn't listening in on this thread and picking it up
himself)?

Tested-by: Paul Moore <paul@paul-moore.com>

>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index d56c0559b477..0ff31d97d485 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -1618,12 +1618,12 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>>               }
>>       }
>>
>> -     if (rcu_access_pointer(sk->sk_filter)) {
>> -             if (udp_lib_checksum_complete(skb))
>> +     if (rcu_access_pointer(sk->sk_filter) &&
>> +         udp_lib_checksum_complete(skb))
>>                       goto csum_error;
>> -             if (sk_filter(sk, skb))
>> -                     goto drop;
>> -     }
>> +
>> +     if (sk_filter(sk, skb))
>> +             goto drop;
>>
>>       udp_csum_pull_header(skb);
>>       if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 2da1896af934..f421c9f23c5b 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -653,12 +653,12 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>>               }
>>       }
>>
>> -     if (rcu_access_pointer(sk->sk_filter)) {
>> -             if (udp_lib_checksum_complete(skb))
>> -                     goto csum_error;
>> -             if (sk_filter(sk, skb))
>> -                     goto drop;
>> -     }
>> +     if (rcu_access_pointer(sk->sk_filter) &&
>> +         udp_lib_checksum_complete(skb))
>> +             goto csum_error;
>> +
>> +     if (sk_filter(sk, skb))
>> +             goto drop;
>>
>>       udp_csum_pull_header(skb);
>>       if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>>
>>

-- 
paul moore
www.paul-moore.com

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Eric Dumazet]

On Thu, 2016-06-02 at 17:36 -0400, Paul Moore wrote:
> On Wed, Jun 1, 2016 at 4:44 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 06/01/2016 03:18 PM, Eric Dumazet wrote:
> >> On Wed, 2016-06-01 at 15:01 -0400, Paul Moore wrote:
> >>> Hello,
> >>>
> >>> I'm currently trying to debug a problem with 4.7-rc1 and labeled
> >>> networking over UDP.  I'm having some difficulty with the latest
> >>> 4.7-rc1 builds on my test system at the moment so I haven't been able
> >>> to concisely identify the problem, but looking through the commits in
> >>> 4.7-rc1 I think there may be a problem with the following:
> >>>
> >>>   commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
> >>>   Author: samanthakumar <samanthakumar@google.com>
> >>>   Date:   Tue Apr 5 12:41:15 2016 -0400
> >>>
> >>>    udp: remove headers from UDP packets before queueing
> >>>
> >>>    Remove UDP transport headers before queueing packets for reception.
> >>>    This change simplifies a follow-up patch to add MSG_PEEK support.
> >>>
> >>>    Signed-off-by: Sam Kumar <samanthakumar@google.com>
> >>>    Signed-off-by: Willem de Bruijn <willemb@google.com>
> >>>    Signed-off-by: David S. Miller <davem@davemloft.net>
> >>>
> >>> ... it appears that this commit changes things so that sk_filter() is
> >>> only called when sk->sk_filter is not NULL.  While this is fine for
> >>> the traditional socket filter case, it causes problems with LSMs that
> >>> make use of security_sock_rcv_skb() to enforce per-packet access
> >>> controls.
> >>>
> >>> Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
> >>> bisection test around this patch, but I wanted to mention this now in
> >>> case others are seeing the same problem.
> >>
> >> Thanks for the report. Please try following fix.
> >>
> >> sk_filter() got additional features like the skb_pfmemalloc() things and
> >> security_sock_rcv_skb()
> >
> > This resolved the SELinux regression for me.
> >
> > Tested-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> The patch works for me too.  Eric, are you going to send this to DaveM
> (assuming he isn't listening in on this thread and picking it up
> himself)?
> 
> Tested-by: Paul Moore <paul@paul-moore.com>

I am going to send the official patch right away, thanks !

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

Paul Moore tracked a regression caused by a recent commit, which
mistakenly assumed that sk_filter() could be avoided if socket
had no current BPF filter.

The intent was to avoid udp_lib_checksum_complete() overhead.

But sk_filter() also checks skb_pfmemalloc() and
security_sock_rcv_skb(), so better call it.

Fixes: e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Paul Moore <paul@paul-moore.com>
Tested-by: Paul Moore <paul@paul-moore.com>
Tested-by: Stephen Smalley <sds@tycho.nsa.gov>
Cc: samanthakumar <samanthakumar@google.com>
---
 net/ipv4/udp.c |   10 +++++-----
 net/ipv6/udp.c |   12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index d56c0559b477..0ff31d97d485 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1618,12 +1618,12 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		}
 	}
 
-	if (rcu_access_pointer(sk->sk_filter)) {
-		if (udp_lib_checksum_complete(skb))
+	if (rcu_access_pointer(sk->sk_filter) &&
+	    udp_lib_checksum_complete(skb))
 			goto csum_error;
-		if (sk_filter(sk, skb))
-			goto drop;
-	}
+
+	if (sk_filter(sk, skb))
+		goto drop;
 
 	udp_csum_pull_header(skb);
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 2da1896af934..f421c9f23c5b 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -653,12 +653,12 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		}
 	}
 
-	if (rcu_access_pointer(sk->sk_filter)) {
-		if (udp_lib_checksum_complete(skb))
-			goto csum_error;
-		if (sk_filter(sk, skb))
-			goto drop;
-	}
+	if (rcu_access_pointer(sk->sk_filter) &&
+	    udp_lib_checksum_complete(skb))
+		goto csum_error;
+
+	if (sk_filter(sk, skb))
+		goto drop;
 
 	udp_csum_pull_header(skb);
 	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Jun 2016 14:52:43 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> Paul Moore tracked a regression caused by a recent commit, which
> mistakenly assumed that sk_filter() could be avoided if socket
> had no current BPF filter.
> 
> The intent was to avoid udp_lib_checksum_complete() overhead.
> 
> But sk_filter() also checks skb_pfmemalloc() and
> security_sock_rcv_skb(), so better call it.
> 
> Fixes: e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Paul Moore <paul@paul-moore.com>
> Tested-by: Paul Moore <paul@paul-moore.com>
> Tested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: samanthakumar <samanthakumar@google.com>

Applied, thanks Eric.

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[Eric Dumazet]

On Thu, 2016-06-02 at 18:31 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 02 Jun 2016 14:52:43 -0700
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Paul Moore tracked a regression caused by a recent commit, which
> > mistakenly assumed that sk_filter() could be avoided if socket
> > had no current BPF filter.
> > 
> > The intent was to avoid udp_lib_checksum_complete() overhead.
> > 
> > But sk_filter() also checks skb_pfmemalloc() and
> > security_sock_rcv_skb(), so better call it.
> > 
> > Fixes: e6afc8ace6dd ("udp: remove headers from UDP packets before queueing")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Reported-by: Paul Moore <paul@paul-moore.com>
> > Tested-by: Paul Moore <paul@paul-moore.com>
> > Tested-by: Stephen Smalley <sds@tycho.nsa.gov>
> > Cc: samanthakumar <samanthakumar@google.com>
> 
> Applied, thanks Eric.

Arg, I totally messed up the patch title :(

Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Jun 2016 19:58:26 -0700

> Arg, I totally messed up the patch title :(

I noticed it was odd, but it's not a big deal.