netdev - Re: [PATCH] ipv4: Get the address of interface correctly.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180128141907.GG13338@ZenIV.linux.org.uk>
Date:   Sun, 28 Jan 2018 14:19:08 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] ipv4: Get the address of interface correctly.

On Sun, Jan 28, 2018 at 03:38:58AM -0800, Tonghao Zhang wrote:
> When using ioctl to get address of interface, we can't
> get it anymore. For example, the command is show as below.
> 
> 	# ifconfig eth0
> 
> In the patch ("03aef17bb79b3"), the devinet_ioctl does not
> return a suitable value, even though we can find it in
> the kernel. Then fix it now.

D'oh...

Acked-by: Al Viro <viro@...iv.linux.org.uk>

I really wonder how has that avoided loud screams at boot time...

Wouldn't it be better to move that ret = 0; in front of the
entire switch, though?  Look:
        ret = -EADDRNOTAVAIL;
        if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS)
                goto done;

	switch (cmd) {
[4 cases where we needed ret = 0; - those used to rely upon copy_to_user()]
                if (colon) {
                        ret = -EADDRNOTAVAIL;
                        if (!ifa)
                                break;
                        ret = 0;
                        if (!(ifr->ifr_flags & IFF_UP))
                                inet_del_ifa(in_dev, ifap, 1);
                        break;
                }
                ret = dev_change_flags(dev, ifr->ifr_flags);
                break;
always overwrites ret 
        case SIOCSIFADDR:       /* Set interface address (and family) */
                ret = -EINVAL;
...
immediately overwrites ret
        case SIOCSIFBRDADDR:    /* Set the broadcast address */
                ret = 0;
...
        case SIOCSIFDSTADDR:    /* Set the destination address */
                ret = 0;
...
        case SIOCSIFNETMASK:    /* Set the netmask for the interface */

                /*
                 *      The mask we set must be legal.
                 */
                ret = -EINVAL;
                if (bad_mask(sin->sin_addr.s_addr, 0))
                        break;
                ret = 0;
...
	}
and we have no cases not dealt with - switch in the beginning has taken care of
validating cmd.

I mean something like this (completely untested)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index e056c0067f2c..617b55558591 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1046,6 +1046,7 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 	if (!ifa && cmd != SIOCSIFADDR && cmd != SIOCSIFFLAGS)
 		goto done;
 
+	ret = 0;
 	switch (cmd) {
 	case SIOCGIFADDR:	/* Get interface address */
 		sin->sin_addr.s_addr = ifa->ifa_local;
@@ -1065,10 +1066,10 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 
 	case SIOCSIFFLAGS:
 		if (colon) {
-			ret = -EADDRNOTAVAIL;
-			if (!ifa)
+			if (unlikely(!ifa)) {
+				ret = -EADDRNOTAVAIL;
 				break;
-			ret = 0;
+			}
 			if (!(ifr->ifr_flags & IFF_UP))
 				inet_del_ifa(in_dev, ifap, 1);
 			break;
@@ -1077,22 +1078,23 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 		break;
 
 	case SIOCSIFADDR:	/* Set interface address (and family) */
-		ret = -EINVAL;
-		if (inet_abc_len(sin->sin_addr.s_addr) < 0)
+		if (unlikely(inet_abc_len(sin->sin_addr.s_addr) < 0)) {
+			ret = -EINVAL;
 			break;
+		}
 
 		if (!ifa) {
-			ret = -ENOBUFS;
 			ifa = inet_alloc_ifa();
-			if (!ifa)
+			if (unlikely(!ifa)) {
+				ret = -ENOBUFS;
 				break;
+			}
 			INIT_HLIST_NODE(&ifa->hash);
 			if (colon)
 				memcpy(ifa->ifa_label, ifr->ifr_name, IFNAMSIZ);
 			else
 				memcpy(ifa->ifa_label, dev->name, IFNAMSIZ);
 		} else {
-			ret = 0;
 			if (ifa->ifa_local == sin->sin_addr.s_addr)
 				break;
 			inet_del_ifa(in_dev, ifap, 0);
@@ -1118,7 +1120,6 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 		break;
 
 	case SIOCSIFBRDADDR:	/* Set the broadcast address */
-		ret = 0;
 		if (ifa->ifa_broadcast != sin->sin_addr.s_addr) {
 			inet_del_ifa(in_dev, ifap, 0);
 			ifa->ifa_broadcast = sin->sin_addr.s_addr;
@@ -1127,13 +1128,12 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 		break;
 
 	case SIOCSIFDSTADDR:	/* Set the destination address */
-		ret = 0;
 		if (ifa->ifa_address == sin->sin_addr.s_addr)
 			break;
-		ret = -EINVAL;
-		if (inet_abc_len(sin->sin_addr.s_addr) < 0)
+		if (unlikely(inet_abc_len(sin->sin_addr.s_addr) < 0)) {
+			ret = -EINVAL;
 			break;
-		ret = 0;
+		}
 		inet_del_ifa(in_dev, ifap, 0);
 		ifa->ifa_address = sin->sin_addr.s_addr;
 		inet_insert_ifa(ifa);
@@ -1144,10 +1144,10 @@ int devinet_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr)
 		/*
 		 *	The mask we set must be legal.
 		 */
-		ret = -EINVAL;
-		if (bad_mask(sin->sin_addr.s_addr, 0))
+		if (unlikely(bad_mask(sin->sin_addr.s_addr, 0))) {
+			ret = -EINVAL;
 			break;
-		ret = 0;
+		}
 		if (ifa->ifa_mask != sin->sin_addr.s_addr) {
 			__be32 old_mask = ifa->ifa_mask;
 			inet_del_ifa(in_dev, ifap, 0);