[PATCH] net: socket: Always initialize family field at move_addr_to_kernel().

[Casey Schaufler]

On 4/11/2019 4:31 AM, Tetsuo Handa wrote:
> On 2019/04/04 13:49, David Miller wrote:
>> From: Tetsuo Handa <penguin-kernel at i-love.sakura.ne.jp>
>> Date: Wed, 3 Apr 2019 06:07:40 +0900
>>
>>> On 2019/04/03 5:23, David Miller wrote:
>>>> Please fix RDS and other protocols to examine the length properly
>>>> instead.
>>> Do you prefer adding branches only for allow reading the family of socket address?
>> If the length is zero, there is no point in reading the family.
>>
> (Adding LSM people.)
>
> syzbot is reporting that RDS is not checking valid length of address given from userspace.
> It turned out that there are several users who access "struct sockaddr"->family without
> checking valid length (which will be reported by KMSAN).
>
> Unfortunately, since tipc_bind() in net/tipc/socket.c accepts length == 0 as a valid input,
> we can't reject length < offsetofend(struct sockaddr, sa_family) with -EINVAL at
> move_addr_to_kernel() which are called from bind()/connect() system calls. I proposed
> always setting "struct sockaddr"->family at move_addr_to_kernel() but David does not
> like such trick.
>
> Therefore, LSM modules which checks address and/or port have to check valid length
> before accessing "struct sockaddr"->family in order to determine IPv4 or IPv6 or UNIX.
>
> Below is all-in-one change (for x86_64 allmodconfig). Well, I've just realized that
> move_addr_to_kernel() is also called by sendmsg() system call and for now added changes
> for only security/ directory. Is this change appropriate for you? (I wish we could
> simplify this by automatically initializing "struct sockaddr_storage" with 0 at
> move_addr_to_kernel()...)
> ---
>   drivers/isdn/mISDN/socket.c |  4 ++--
>   net/bluetooth/sco.c         |  4 ++--
>   net/core/filter.c           |  2 ++
>   net/ipv6/udp.c              |  2 ++
>   net/llc/af_llc.c            |  3 +--
>   net/netlink/af_netlink.c    |  3 ++-
>   net/rds/af_rds.c            |  3 +++
>   net/rds/bind.c              |  2 ++
>   net/rxrpc/af_rxrpc.c        |  3 ++-
>   net/sctp/socket.c           |  3 ++-
>   security/apparmor/lsm.c     |  6 ++++++
>   security/selinux/hooks.c    | 12 ++++++++++++
>   security/smack/smack_lsm.c  | 39 +++++++++++++++++++++++++++++++++++----
>   security/tomoyo/network.c   | 12 ++++++++++++
>   14 files changed, 85 insertions(+), 13 deletions(-)

Except as noted below this looks fine for Smack.

> ...

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c16135..7c19c04 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2805,13 +2805,21 @@ static int smack_socket_socketpair(struct socket *socka,
>    *
>    * Records the label bound to a port.
>    *
> - * Returns 0
> + * Returns 0 on success, and error code otherwise
>    */
>   static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
>   				int addrlen)
>   {
> -	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6)
> +	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
> +		/*
> +		 * Reject if valid length is too short for IPv6 address or
> +		 * address family is not IPv6.
> +		 */
> +		if (addr_len < SIN6_LEN_RFC2133 ||

  +		if (addrlen < SIN6_LEN_RFC2133 ||
  

> +		    address->sa_family != AF_INET6)
> +			return -EINVAL;
>   		smk_ipv6_port_label(sock, address);
> +	}
>   	return 0;
>   }
>   #endif /* SMACK_IPV6_PORT_LABELING */
> @@ -2847,12 +2855,21 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>   
>   	switch (sock->sk->sk_family) {
>   	case PF_INET:
> -		if (addrlen < sizeof(struct sockaddr_in))
> +		/*
> +		 * Reject if valid length is too short for IPv4 address or
> +		 * address family is not IPv4.
> +		 */
> +		if (addrlen < sizeof(struct sockaddr_in) ||
> +		    sap->sa_family != AF_INET)
>   			return -EINVAL;
>   		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
>   		break;
>   	case PF_INET6:
> -		if (addrlen < sizeof(struct sockaddr_in6))
> +		/*
> +		 * Reject if valid length is too short for IPv6 address or
> +		 * address family is not IPv6.
> +		 */
> +		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
>   			return -EINVAL;
>   #ifdef SMACK_IPV6_SECMARK_LABELING
>   		rsp = smack_ipv6host_label(sip);
> @@ -3682,9 +3699,23 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
>   
>   	switch (sock->sk->sk_family) {
>   	case AF_INET:
> +		/*
> +		 * Reject if valid length is too short for IPv4 address or
> +		 * address family is not IPv4.
> +		 */
> +		if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> +		    sip->sin_family != AF_INET)
> +			return -EINVAL;
>   		rc = smack_netlabel_send(sock->sk, sip);
>   		break;
>   	case AF_INET6:
> +		/*
> +		 * Reject if valid length is too short for IPv6 address or
> +		 * address family is not IPv6.
> +		 */
> +		if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
> +		    sap->sin6_family != AF_INET6)
> +			return -EINVAL;
>   #ifdef SMACK_IPV6_SECMARK_LABELING
>   		rsp = smack_ipv6host_label(sap);
>   		if (rsp != NULL)
> diff --git a/security/tomoyo/network.c b/security/tomoyo/network.c
> index 9094f4b..3cbd6bd 100644
> --- a/security/tomoyo/network.c
> +++ b/security/tomoyo/network.c
> @@ -505,6 +505,12 @@ static int tomoyo_check_inet_address(const struct sockaddr *addr,
>   {
>   	struct tomoyo_inet_addr_info *i = &address->inet;
>   
> +	/*
> +	 * Nothing more to do if valid length is too short to check
> +	 * address->sa_family.
> +	 */
> +	if (addr_len < offsetofend(struct sockaddr, sa_family))
> +		return 0;
>   	switch (addr->sa_family) {
>   	case AF_INET6:
>   		if (addr_len < SIN6_LEN_RFC2133)
> @@ -594,6 +600,12 @@ static int tomoyo_check_unix_address(struct sockaddr *addr,
>   {
>   	struct tomoyo_unix_addr_info *u = &address->unix0;
>   
> +	/*
> +	 * Nothing more to do if valid length is too short to check
> +	 * address->sa_family.
> +	 */
> +	if (addr_len < offsetofend(struct sockaddr, sa_family))
> +		return 0;
>   	if (addr->sa_family != AF_UNIX)
>   		return 0;
>   	u->addr = ((struct sockaddr_un *) addr)->sun_path;