commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

I am seeing ping failures to IPv6 linklocal addresses with Debian
buster. Easiest example to reproduce is:

$ ping -c1 -w1 ff02::1%eth1
connect: Invalid argument

$ ping -c1 -w1 ff02::1%eth1
PING ff02::01%eth1(ff02::1%eth1) 56 data bytes
64 bytes from fe80::e0:f9ff:fe0c:37%eth1: icmp_seq=1 ttl=64 time=0.059 ms


git bisect traced the failure to:

commit b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date:   Fri Apr 12 19:59:35 2019 +0900

    smack: Check address length before reading address family


Arguably ping is being stupid since the buster version is not setting
the address family properly (ping on stretch for example does):

$ strace -e connect ping6 -c1 -w1 ff02::1%eth1
connect(4, {sa_family=AF_UNSPEC,
sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28) = 0

but the command works fine on kernels prior to this commit, so this is
breakage which goes against the Linux paradigm of "don't break userspace"

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Tetsuo Handa]

Hello David,

Thank you for reporting. Will you confirm that this patch fixes your problem?
----------------------------------------
From 06a61125a79139941cdebee3a24b0b4bed576742 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 6 Jan 2020 12:46:49 +0900
Subject: [PATCH] smack: Don't reject IPv6's bind() when socket family is invalid

David Ahern has reported that commit b9ef5513c99bf9c8 ("smack: Check
address length before reading address family") breaks ping program in
Debian Buster because that version of ping program is by error using
AF_UNSPEC instead of AF_INET6 when calling connect().

Since rawv6_bind() will fail with -EINVAL and __inet6_bind() from
inet6_bind() will fail with -EAFNOSUPPORT if sin6_family != AF_INET6,
smack_socket_bind() can return 0 rather than -EINVAL.

Reported-by: David Ahern <dsahern@gmail.com>
Bisected-by: David Ahern <dsahern@gmail.com>
Fixes: b9ef5513c99bf9c8 ("smack: Check address length before reading address family")
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: stable@vger.kernel.org
---
 security/smack/smack_lsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ecea41ce919b..5b2177724d5e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2810,7 +2810,7 @@ static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
 	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
 		if (addrlen < SIN6_LEN_RFC2133 ||
 		    address->sa_family != AF_INET6)
-			return -EINVAL;
+			return 0;
 		smk_ipv6_port_label(sock, address);
 	}
 	return 0;
-- 
2.16.6

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/5/20 8:51 PM, Tetsuo Handa wrote:
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ecea41ce919b..5b2177724d5e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2810,7 +2810,7 @@ static int smack_socket_bind(struct socket *sock, struct sockaddr *address,
>  	if (sock->sk != NULL && sock->sk->sk_family == PF_INET6) {
>  		if (addrlen < SIN6_LEN_RFC2133 ||
>  		    address->sa_family != AF_INET6)
> -			return -EINVAL;
> +			return 0;
>  		smk_ipv6_port_label(sock, address);
>  	}
>  	return 0;
> 

Hi:

The failure is the connect function, not the bind.

This change seems more appropriate to me (and fixes the failure):

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ecea41ce919b..ce5e3be7c111 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2854,7 +2854,7 @@ static int smack_socket_connect(struct socket
*sock, struct sockaddr *sap,
                rc = smack_netlabel_send(sock->sk, (struct sockaddr_in
*)sap);
                break;
        case PF_INET6:
-               if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family !=
AF_INET6)
+               if (addrlen < SIN6_LEN_RFC2133)
                        return -EINVAL;
 #ifdef SMACK_IPV6_SECMARK_LABELING
                rsp = smack_ipv6host_label(sip);


ie., if the socket family is AF_INET6 the address length should be an
IPv6 address. The family in the sockaddr is not as important.

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Tetsuo Handa]

On 2020/01/06 13:20, David Ahern wrote:
> Hi:
> 
> The failure is the connect function, not the bind.

Oops, I missed it.

> 
> This change seems more appropriate to me (and fixes the failure):
> 
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index ecea41ce919b..ce5e3be7c111 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2854,7 +2854,7 @@ static int smack_socket_connect(struct socket
> *sock, struct sockaddr *sap,
>                 rc = smack_netlabel_send(sock->sk, (struct sockaddr_in
> *)sap);
>                 break;
>         case PF_INET6:
> -               if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family !=
> AF_INET6)
> +               if (addrlen < SIN6_LEN_RFC2133)
>                         return -EINVAL;

This is called upon connect(), isn't it? Then, it is possible that a socket's
protocol family is PF_INET6 but address given is AF_INET, isn't it? For example,
__ip6_datagram_connect() checks for AF_INET before checking addrlen is at least
SIN6_LEN_RFC2133 bytes. Thus, I think that we need to return 0 if address given
is AF_INET even if socket is PF_INET6.

>  #ifdef SMACK_IPV6_SECMARK_LABELING
>                 rsp = smack_ipv6host_label(sip);
> 
> 
> ie., if the socket family is AF_INET6 the address length should be an
> IPv6 address. The family in the sockaddr is not as important.
> 

Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/6/20 4:04 AM, Tetsuo Handa wrote:
>>
>> This change seems more appropriate to me (and fixes the failure):
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index ecea41ce919b..ce5e3be7c111 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -2854,7 +2854,7 @@ static int smack_socket_connect(struct socket
>> *sock, struct sockaddr *sap,
>>                 rc = smack_netlabel_send(sock->sk, (struct sockaddr_in
>> *)sap);
>>                 break;
>>         case PF_INET6:
>> -               if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family !=
>> AF_INET6)
>> +               if (addrlen < SIN6_LEN_RFC2133)
>>                         return -EINVAL;
> 
> This is called upon connect(), isn't it? Then, it is possible that a socket's
> protocol family is PF_INET6 but address given is AF_INET, isn't it? For example,
> __ip6_datagram_connect() checks for AF_INET before checking addrlen is at least
> SIN6_LEN_RFC2133 bytes. Thus, I think that we need to return 0 if address given
> is AF_INET even if socket is PF_INET6.

In this case, the sockaddr is AF_UNSPEC. From the first message:

$ strace -e connect ping6 -c1 -w1 ff02::1%eth1
connect(4, {sa_family=AF_UNSPEC,
sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28) = 0

That's why I was suggesting if the socket is PF_INET6 and the address
length is at least SIN6_LEN_RFC2133, then don't worry about the sa_family.


> 
>>  #ifdef SMACK_IPV6_SECMARK_LABELING
>>                 rsp = smack_ipv6host_label(sip);
>>
>>
>> ie., if the socket family is AF_INET6 the address length should be an
>> IPv6 address. The family in the sockaddr is not as important.
>>
> 
> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
> 

not sure. I have not seen a problem related to it yet.

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Tetsuo Handa]

On 2020/01/07 1:41, David Ahern wrote:
>>>  #ifdef SMACK_IPV6_SECMARK_LABELING
>>>                 rsp = smack_ipv6host_label(sip);
>>>
>>>
>>> ie., if the socket family is AF_INET6 the address length should be an
>>> IPv6 address. The family in the sockaddr is not as important.
>>>
>>
>> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
>>
> 
> not sure. I have not seen a problem related to it yet.
> 

A sample program shown below is expected to return 0.
Casey, what does smack wants to do for IPv4 address on IPv6 socket case?

----------
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <arpa/inet.h>
#include <unistd.h>

int main(int argc, char *argv[])
{
        const int fd1 = socket(PF_INET6, SOCK_DGRAM, 0);
        const int fd2 = socket(PF_INET, SOCK_DGRAM, 0);
        struct sockaddr_in addr1 = {
                .sin_family = AF_INET,
                .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
                .sin_port = htons(10000)
        };
        struct sockaddr_in addr2 = { };
        char c = 0;
        struct iovec iov1 = { "", 1 };
        struct iovec iov2 = { &c, 1 };
        const struct msghdr msg1 = {
                .msg_iov = &iov1,
                .msg_iovlen = 1,
                .msg_name = &addr1,
                .msg_namelen = sizeof(addr1)
        };
        struct msghdr msg2 = {
                .msg_iov = &iov2,
                .msg_iovlen = 1,
                .msg_name = &addr2,
                .msg_namelen = sizeof(addr2)
        };
        if (bind(fd2, (struct sockaddr *) &addr1, sizeof(addr1)))
                return 1;
        if (sendmsg(fd1, &msg1, 0) != 1 || recvmsg(fd2, &msg2, 0) != 1)
                return 1;
        if (connect(fd1, (struct sockaddr *) &addr1, sizeof(addr1)))
                return 1;
        if (send(fd1, "", 1, 0) != 1 || recv(fd2, &c, 1, 0) != 1)
                return 1;
        return 0;
}
----------

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Casey Schaufler]

On 1/6/2020 1:03 PM, Tetsuo Handa wrote:
> On 2020/01/07 1:41, David Ahern wrote:
>>>>  #ifdef SMACK_IPV6_SECMARK_LABELING
>>>>                 rsp = smack_ipv6host_label(sip);
>>>>
>>>>
>>>> ie., if the socket family is AF_INET6 the address length should be an
>>>> IPv6 address. The family in the sockaddr is not as important.
>>>>
>>> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
>>>
>> not sure. I have not seen a problem related to it yet.
>>
> A sample program shown below is expected to return 0.
> Casey, what does smack wants to do for IPv4 address on IPv6 socket case?

I have to take a minute to look more closely at what's involved.


>
> ----------
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <arpa/inet.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
>         const int fd1 = socket(PF_INET6, SOCK_DGRAM, 0);
>         const int fd2 = socket(PF_INET, SOCK_DGRAM, 0);
>         struct sockaddr_in addr1 = {
>                 .sin_family = AF_INET,
>                 .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
>                 .sin_port = htons(10000)
>         };
>         struct sockaddr_in addr2 = { };
>         char c = 0;
>         struct iovec iov1 = { "", 1 };
>         struct iovec iov2 = { &c, 1 };
>         const struct msghdr msg1 = {
>                 .msg_iov = &iov1,
>                 .msg_iovlen = 1,
>                 .msg_name = &addr1,
>                 .msg_namelen = sizeof(addr1)
>         };
>         struct msghdr msg2 = {
>                 .msg_iov = &iov2,
>                 .msg_iovlen = 1,
>                 .msg_name = &addr2,
>                 .msg_namelen = sizeof(addr2)
>         };
>         if (bind(fd2, (struct sockaddr *) &addr1, sizeof(addr1)))
>                 return 1;
>         if (sendmsg(fd1, &msg1, 0) != 1 || recvmsg(fd2, &msg2, 0) != 1)
>                 return 1;
>         if (connect(fd1, (struct sockaddr *) &addr1, sizeof(addr1)))
>                 return 1;
>         if (send(fd1, "", 1, 0) != 1 || recv(fd2, &c, 1, 0) != 1)
>                 return 1;
>         return 0;
> }
> ----------

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Casey Schaufler]

On 1/6/2020 1:03 PM, Tetsuo Handa wrote:
> On 2020/01/07 1:41, David Ahern wrote:
>>>>  #ifdef SMACK_IPV6_SECMARK_LABELING
>>>>                 rsp = smack_ipv6host_label(sip);
>>>>
>>>>
>>>> ie., if the socket family is AF_INET6 the address length should be an
>>>> IPv6 address. The family in the sockaddr is not as important.
>>>>
>>> Commit b9ef5513c99b was wrong, but we need to also fix commit c673944347ed ?
>>>
>> not sure. I have not seen a problem related to it yet.
>>
> A sample program shown below is expected to return 0.
> Casey, what does smack wants to do for IPv4 address on IPv6 socket case?

Thank you, this program has been very helpful. The problematic
checks are supposed to be simply for data sanity, not security.
I think I've got the right set of checks figured out. I'll send
a patch for review once it tests out.

>
> ----------
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <arpa/inet.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
>         const int fd1 = socket(PF_INET6, SOCK_DGRAM, 0);
>         const int fd2 = socket(PF_INET, SOCK_DGRAM, 0);
>         struct sockaddr_in addr1 = {
>                 .sin_family = AF_INET,
>                 .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
>                 .sin_port = htons(10000)
>         };
>         struct sockaddr_in addr2 = { };
>         char c = 0;
>         struct iovec iov1 = { "", 1 };
>         struct iovec iov2 = { &c, 1 };
>         const struct msghdr msg1 = {
>                 .msg_iov = &iov1,
>                 .msg_iovlen = 1,
>                 .msg_name = &addr1,
>                 .msg_namelen = sizeof(addr1)
>         };
>         struct msghdr msg2 = {
>                 .msg_iov = &iov2,
>                 .msg_iovlen = 1,
>                 .msg_name = &addr2,
>                 .msg_namelen = sizeof(addr2)
>         };
>         if (bind(fd2, (struct sockaddr *) &addr1, sizeof(addr1)))
>                 return 1;
>         if (sendmsg(fd1, &msg1, 0) != 1 || recvmsg(fd2, &msg2, 0) != 1)
>                 return 1;
>         if (connect(fd1, (struct sockaddr *) &addr1, sizeof(addr1)))
>                 return 1;
>         if (send(fd1, "", 1, 0) != 1 || recv(fd2, &c, 1, 0) != 1)
>                 return 1;
>         return 0;
> }
> ----------

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Casey Schaufler]

Does this patch address the Debian issue? It works for the test program
and on my Fedora system.


 security/smack/smack_lsm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 50c536cad85b..b0bb36419aeb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
 		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
 		break;
 	case PF_INET6:
-		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
+		if (addrlen < SIN6_LEN_RFC2133)
+			return 0;
+		if (sap->sa_family != AF_INET6)
 			return -EINVAL;
 #ifdef SMACK_IPV6_SECMARK_LABELING
 		rsp = smack_ipv6host_label(sip);
@@ -3694,8 +3696,9 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
-		    sap->sin6_family != AF_INET6)
+		if (msg->msg_namelen < SIN6_LEN_RFC2133)
+			return 0;
+		if (sap->sin6_family != AF_INET6)
 			return -EINVAL;
 #ifdef SMACK_IPV6_SECMARK_LABELING
 		rsp = smack_ipv6host_label(sap);

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/7/20 11:40 AM, Casey Schaufler wrote:
> Does this patch address the Debian issue? It works for the test program
> and on my Fedora system.
> 
> 
>  security/smack/smack_lsm.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 50c536cad85b..b0bb36419aeb 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>  		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
>  		break;
>  	case PF_INET6:
> -		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
> +		if (addrlen < SIN6_LEN_RFC2133)
> +			return 0;
> +		if (sap->sa_family != AF_INET6)
>  			return -EINVAL;

I doubt it; can't test it until tonight. strace for ping on buster is
showing this:

$ strace -e connect ping6 -c1 -w1 ff02::1%eth1
connect(4, {sa_family=AF_UNSPEC,
sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28)

ie., the addrlen >= SIN6_LEN_RFC2133 but the family is AF_UNSPEC. That
suggests to me the first check passes and the second check fails.

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/7/20 11:44 AM, David Ahern wrote:
> On 1/7/20 11:40 AM, Casey Schaufler wrote:
>> Does this patch address the Debian issue? It works for the test program
>> and on my Fedora system.
>>
>>
>>  security/smack/smack_lsm.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 50c536cad85b..b0bb36419aeb 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -2857,7 +2857,9 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
>>  		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
>>  		break;
>>  	case PF_INET6:
>> -		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
>> +		if (addrlen < SIN6_LEN_RFC2133)
>> +			return 0;
>> +		if (sap->sa_family != AF_INET6)
>>  			return -EINVAL;
> 
> I doubt it; can't test it until tonight. strace for ping on buster is
> showing this:
> 
> $ strace -e connect ping6 -c1 -w1 ff02::1%eth1
> connect(4, {sa_family=AF_UNSPEC,
> sa_data="\4\1\0\0\0\0\377\2\0\0\0\0\0\0\0\0\0\0\0\0\0\1\3\0\0\0"}, 28)
> 
> ie., the addrlen >= SIN6_LEN_RFC2133 but the family is AF_UNSPEC. That
> suggests to me the first check passes and the second check fails.
> 

confirmed. connect still fails EINVAL.

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Casey Schaufler]

This version should work, I think. Please verify. Thank you.

----
 security/smack/smack_lsm.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 50c536cad85b..75b3953212e2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2857,10 +2857,13 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
 		rc = smack_netlabel_send(sock->sk, (struct sockaddr_in *)sap);
 		break;
 	case PF_INET6:
-		if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family != AF_INET6)
-			return -EINVAL;
+		if (addrlen < SIN6_LEN_RFC2133)
+			return 0;
 #ifdef SMACK_IPV6_SECMARK_LABELING
-		rsp = smack_ipv6host_label(sip);
+		if (sap->sa_family != AF_INET6)
+			rsp = NULL;
+		else
+			rsp = smack_ipv6host_label(sip);
 		if (rsp != NULL)
 			rc = smk_ipv6_check(ssp->smk_out, rsp, sip,
 						SMK_CONNECTING);
@@ -3694,11 +3697,13 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
 	case AF_INET6:
-		if (msg->msg_namelen < SIN6_LEN_RFC2133 ||
-		    sap->sin6_family != AF_INET6)
-			return -EINVAL;
+		if (msg->msg_namelen < SIN6_LEN_RFC2133)
+			return 0;
 #ifdef SMACK_IPV6_SECMARK_LABELING
-		rsp = smack_ipv6host_label(sap);
+		if (sap->sin6_family != AF_INET6)
+			rsp = NULL;
+		else
+			rsp = smack_ipv6host_label(sap);
 		if (rsp != NULL)
 			rc = smk_ipv6_check(ssp->smk_out, rsp, sap,
 						SMK_CONNECTING);

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/8/20 6:53 PM, 吉藤英明 wrote:
> Hi,
> 
> 2020年1月9日(木) 8:06 Casey Schaufler <casey@schaufler-ca.com
> <mailto:casey@schaufler-ca.com>>:
> 
>     This version should work, I think. Please verify. Thank you.
> 
>     ----
>      security/smack/smack_lsm.c | 19 ++++++++++++-------
>      1 file changed, 12 insertions(+), 7 deletions(-)
> 
>     diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>     index 50c536cad85b..75b3953212e2 100644
>     --- a/security/smack/smack_lsm.c
>     +++ b/security/smack/smack_lsm.c
>     @@ -2857,10 +2857,13 @@ static int smack_socket_connect(struct
>     socket *sock, struct sockaddr *sap,
>                     rc = smack_netlabel_send(sock->sk, (struct
>     sockaddr_in *)sap);
>                     break;
>             case PF_INET6:
>     -               if (addrlen < SIN6_LEN_RFC2133 || sap->sa_family !=
>     AF_INET6)
>     -                       return -EINVAL;
>     +               if (addrlen < SIN6_LEN_RFC2133)
>     +                       return 0;
> 
> 
> Why don't we assume AF_UNSPEC as if it were AF_INET6 for AF_INET6
> sockets here?
>  

it is not called out explicitly, but the rest of the proposed change:

 #ifdef SMACK_IPV6_SECMARK_LABELING
-		rsp = smack_ipv6host_label(sip);
+		if (sap->sa_family != AF_INET6)
+			rsp = NULL;
+		else
+			rsp = smack_ipv6host_label(sip);
 		if (rsp != NULL)
 			rc = smk_ipv6_check(ssp->smk_out, rsp, sip,
 						SMK_CONNECTING);

seems to handle AF_UNSPEC. Did you have something else in mind?

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[David Ahern]

On 1/8/20 4:06 PM, Casey Schaufler wrote:
> This version should work, I think. Please verify. Thank you.
> 

It does.

Re: commit b9ef5513c99b breaks ping to ipv6 linklocal addresses on debian buster

[Tetsuo Handa]

On 2020/01/09 13:06, David Ahern wrote:
> On 1/8/20 4:06 PM, Casey Schaufler wrote:
>> This version should work, I think. Please verify. Thank you.
>>
> 
> It does.
> 

Don't we want to do that check inside smk_ipv6_check() because
smk_ipv6_port_check() might call smk_ipv6_check() ?

smack does not want to call smack_netlabel_send() (as it is IPv4 socket)
when IPv4 address was given on IPv6 socket, does it?

Also, please fold smack_socket_bind() change (make it no-op unless
valid IPv6 address is given) into your patch.