[PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send

[PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send

[Soheil Hassas Yeganeh]

From: Soheil Hassas Yeganeh <soheil@google.com>

SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
receive-side control messages from recvmsg(). Although invalid,
some applications may reflect those receive-side control messages
back to sendmsg(). Since socket-level control messages were being
ignored in ipv4 and ipv6, such applications would not get an error.

24025c4 (ipv4: process socket-level control messages in IPv4) and
ad1e46 (ipv6: process socket-level control messages in IPv6) add
support for socket-level control messages in ipv4 and ipv6 on
sendmsg(). This results in getting -EINVAL, if the application
passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
and/or SO_TIMESTAMPNS that might have been received in recvmsg().

Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
processing socket-level control messages in send-side to remain
backward compatible.
---
 net/core/sock.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index 08bf97e..1e0bcd0 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1938,6 +1938,12 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
 		sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
 		sockc->tsflags |= tsflags;
 		break;
+	/* Ignore the following types on send to remain backward compatible. */
+	case SO_RXQ_OVFL:	/* Fall through */
+	case SO_TIMESTAMP:	/* Fall through */
+	case SO_TIMESTAMPNS:	/* Fall through */
+	case SO_WIFI_STATUS:
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send

[David Miller]

From: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
Date: Fri, 13 May 2016 00:47:10 -0400

> From: Soheil Hassas Yeganeh <soheil@google.com>
> 
> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
> receive-side control messages from recvmsg(). Although invalid,
> some applications may reflect those receive-side control messages
> back to sendmsg(). Since socket-level control messages were being
> ignored in ipv4 and ipv6, such applications would not get an error.
> 
> 24025c4 (ipv4: process socket-level control messages in IPv4) and
> ad1e46 (ipv6: process socket-level control messages in IPv6) add
> support for socket-level control messages in ipv4 and ipv6 on
> sendmsg(). This results in getting -EINVAL, if the application
> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
> 
> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
> processing socket-level control messages in send-side to remain
> backward compatible.

This patch is missing a proper Signed-Off-By: tag.

But I think this change is wrong.  Just because we silently accepted
garbage in the past doesn't mean more strict checking is invalid.

Applications blindly echoing control messages from recvmsg to sendmsg
must be fixed.

Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send

[Soheil Hassas Yeganeh]

On Fri, May 13, 2016 at 2:03 AM, David Miller <davem@davemloft.net> wrote:
> From: Soheil Hassas Yeganeh <soheil.kdev@gmail.com>
> Date: Fri, 13 May 2016 00:47:10 -0400
>
>> From: Soheil Hassas Yeganeh <soheil@google.com>
>>
>> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
>> receive-side control messages from recvmsg(). Although invalid,
>> some applications may reflect those receive-side control messages
>> back to sendmsg(). Since socket-level control messages were being
>> ignored in ipv4 and ipv6, such applications would not get an error.
>>
>> 24025c4 (ipv4: process socket-level control messages in IPv4) and
>> ad1e46 (ipv6: process socket-level control messages in IPv6) add
>> support for socket-level control messages in ipv4 and ipv6 on
>> sendmsg(). This results in getting -EINVAL, if the application
>> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
>> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
>>
>> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
>> processing socket-level control messages in send-side to remain
>> backward compatible.
>
> This patch is missing a proper Signed-Off-By: tag.

Oops, sorry. :(

> But I think this change is wrong.  Just because we silently accepted
> garbage in the past doesn't mean more strict checking is invalid.
>
> Applications blindly echoing control messages from recvmsg to sendmsg
> must be fixed.

Agreed, by this patch, I just wanted to note such potential issues and
make sure I haven't broken any user-space application. Thanks David!

Re: [PATCH net-next] sock: ignore TIMESTAMP, RXQ_OVFL, WIFI_STATUS in sock_cmsg_send

[Sergei Shtylyov]

Hello.

On 5/13/2016 7:47 AM, Soheil Hassas Yeganeh wrote:

> From: Soheil Hassas Yeganeh <soheil@google.com>
>
> SO_TIMESTAMP(NS), RXQ_OVFL, and WIFI_STATUS can be returned as
> receive-side control messages from recvmsg(). Although invalid,
> some applications may reflect those receive-side control messages
> back to sendmsg(). Since socket-level control messages were being
> ignored in ipv4 and ipv6, such applications would not get an error.
>
> 24025c4 (ipv4: process socket-level control messages in IPv4) and
> ad1e46 (ipv6: process socket-level control messages in IPv6) add

    scripts/checkpatch.pl now enforces certain commit citing style, yours 
doesn't quite match -- SHA1 should be at least 12 digits and the summary 
enclosed in ("").

> support for socket-level control messages in ipv4 and ipv6 on
> sendmsg(). This results in getting -EINVAL, if the application
> passes in a message with SO_WIFI_STATUS, SO_RXQ_OVFL, SO_TIMESTAMP
> and/or SO_TIMESTAMPNS that might have been received in recvmsg().
>
> Ignore SO_WIFI_STATUS, SO_TIMESTAMP(NS), and SO_RXQ_OVFL when
> processing socket-level control messages in send-side to remain
> backward compatible.
> ---
>  net/core/sock.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 08bf97e..1e0bcd0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1938,6 +1938,12 @@ int __sock_cmsg_send(struct sock *sk, struct msghdr *msg, struct cmsghdr *cmsg,
>  		sockc->tsflags &= ~SOF_TIMESTAMPING_TX_RECORD_MASK;
>  		sockc->tsflags |= tsflags;
>  		break;
> +	/* Ignore the following types on send to remain backward compatible. */
> +	case SO_RXQ_OVFL:	/* Fall through */
> +	case SO_TIMESTAMP:	/* Fall through */
> +	case SO_TIMESTAMPNS:	/* Fall through */
                                 ^^^^^^^^^^^^^^^^^^
    There's no need for such comments here.

> +	case SO_WIFI_STATUS:
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

MBR, Sergei

[PATCH net-next] sock: propagate __sock_cmsg_send() error

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

__sock_cmsg_send() might return different error codes, not only -EINVAL.

Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
---
 net/ipv4/ip_sockglue.c |    5 +++--
 net/ipv6/datagram.c    |    5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 5805762d7fc79a702f1d67e802992b822f66f000..71a52f4d4cffba2db9353f43dc817689bf4fab10 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -247,8 +247,9 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
 		}
 #endif
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			if (__sock_cmsg_send(sk, msg, cmsg, &ipc->sockc))
-				return -EINVAL;
+			err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
+			if (err)
+				return err;
 			continue;
 		}
 
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 00d0c2903173a96571983216f2839a93059cad22..37874e2f30edf98f31e2a5097761143d507d5b95 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -746,8 +746,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 		}
 
 		if (cmsg->cmsg_level == SOL_SOCKET) {
-			if (__sock_cmsg_send(sk, msg, cmsg, sockc))
-				return -EINVAL;
+			err = __sock_cmsg_send(sk, msg, cmsg, sockc);
+			if (err)
+				return err;
 			continue;
 		}
 

Re: [PATCH net-next] sock: propagate __sock_cmsg_send() error

[Soheil Hassas Yeganeh]

On Fri, May 13, 2016 at 9:14 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> __sock_cmsg_send() might return different error codes, not only -EINVAL.
>
> Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
> Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
> ---
>  net/ipv4/ip_sockglue.c |    5 +++--
>  net/ipv6/datagram.c    |    5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 5805762d7fc79a702f1d67e802992b822f66f000..71a52f4d4cffba2db9353f43dc817689bf4fab10 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -247,8 +247,9 @@ int ip_cmsg_send(struct sock *sk, struct msghdr *msg, struct ipcm_cookie *ipc,
>                 }
>  #endif
>                 if (cmsg->cmsg_level == SOL_SOCKET) {
> -                       if (__sock_cmsg_send(sk, msg, cmsg, &ipc->sockc))
> -                               return -EINVAL;
> +                       err = __sock_cmsg_send(sk, msg, cmsg, &ipc->sockc);
> +                       if (err)
> +                               return err;
>                         continue;
>                 }
>
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 00d0c2903173a96571983216f2839a93059cad22..37874e2f30edf98f31e2a5097761143d507d5b95 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -746,8 +746,9 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
>                 }
>
>                 if (cmsg->cmsg_level == SOL_SOCKET) {
> -                       if (__sock_cmsg_send(sk, msg, cmsg, sockc))
> -                               return -EINVAL;
> +                       err = __sock_cmsg_send(sk, msg, cmsg, sockc);
> +                       if (err)
> +                               return err;
>                         continue;
>                 }
>
>
>

Thanks Eric for the fixes.

Re: [PATCH net-next] sock: propagate __sock_cmsg_send() error

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 13 May 2016 06:14:37 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> __sock_cmsg_send() might return different error codes, not only -EINVAL.
> 
> Fixes: 24025c465f77 ("ipv4: process socket-level control messages in IPv4")
> Fixes: ad1e46a83716 ("ipv6: process socket-level control messages in IPv6")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>

Applied.