| 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
| ||
|
Message-ID: <46F0072C.1080507@candelatech.com>
Date: Tue, 18 Sep 2007 10:13:16 -0700
From: Ben Greear <greearb@...delatech.com>
To: David Miller <davem@...emloft.net>
CC: netdev@...r.kernel.org, kaber@...sh.net
Subject: Re: [PATCH]: New SO_BINDTODEVICE fix.
David Miller wrote:
> Ok, I changed my mind and decided to retain the optlen==0
> intended behavior. It fell out of fixing the small
> string length case.
>
> This is likely what I'll push to Linus and later -stable
> as a fix for this stuff.
I manually patched this into 2.6.20 and ran some tests.
My test program is here:
http://www.candelatech.com/oss/bind_test.c
First, I could not detect any packets routed in-correctly with the
old code that did not release the route. I'm not sure if my
test is bogus or if the old code somehow managed to trigger
a new route lookup anyway.
On failure, I expected the first un-bind to not actually work
and that I would continue to see the second set of 5 packets on
the previously bound interface. I did not see these packets, so
the un-bind worked.
I did test that the new code works with passing "" and 0 length
argument to the un-bind. As far as I can tell, it now matches
the man page and all is well.
Since I could not reproduce a functional error, and you can work around
the man-page mismatch by passing a 4+ byte string of /0 to un-bind, this
may not be required for stable, but it should not hurt.
Thanks,
Ben
>
> Thanks.
>
> commit 4878809f711981a602cc562eb47994fc81ea0155
> Author: David S. Miller <davem@...set.davemloft.net>
> Date: Fri Sep 14 16:41:03 2007 -0700
>
> [NET]: Fix two issues wrt. SO_BINDTODEVICE.
>
> 1) Comments suggest that setting optlen to zero will unbind
> the socket from whatever device it might be attached to. This
> hasn't been the case since at least 2.2.x because the first thing
> this function does is return -EINVAL if 'optlen' is less than
> sizeof(int).
>
> This check also means that passing in a two byte string doesn't
> work so well. It's almost as if this code was testing with "eth?"
> patterned strings and nothing else :-)
>
> Fix this by breaking the logic of this facility out into a
> seperate function which validates optlen more appropriately.
>
> The optlen==0 and small string cases now work properly.
>
> 2) We should reset the cached route of the socket after we have made
> the device binding changes, not before.
>
> Reported by Ben Greear.
>
> Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index cfed7d4..190de61 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -362,6 +362,61 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie)
> }
> EXPORT_SYMBOL(sk_dst_check);
>
> +static int sock_bindtodevice(struct sock *sk, char __user *optval, int optlen)
> +{
> + int ret = -ENOPROTOOPT;
> +#ifdef CONFIG_NETDEVICES
> + char devname[IFNAMSIZ];
> + int index;
> +
> + /* Sorry... */
> + ret = -EPERM;
> + if (!capable(CAP_NET_RAW))
> + goto out;
> +
> + ret = -EINVAL;
> + if (optlen < 0)
> + goto out;
> +
> + /* Bind this socket to a particular device like "eth0",
> + * as specified in the passed interface name. If the
> + * name is "" or the option length is zero the socket
> + * is not bound.
> + */
> + if (optlen > IFNAMSIZ - 1)
> + optlen = IFNAMSIZ - 1;
> + memset(devname, 0, sizeof(devname));
> +
> + ret = -EFAULT;
> + if (copy_from_user(devname, optval, optlen))
> + goto out;
> +
> + if (devname[0] == '\0') {
> + index = 0;
> + } else {
> + struct net_device *dev = dev_get_by_name(devname);
> +
> + ret = -ENODEV;
> + if (!dev)
> + goto out;
> +
> + index = dev->ifindex;
> + dev_put(dev);
> + }
> +
> + lock_sock(sk);
> + sk->sk_bound_dev_if = index;
> + sk_dst_reset(sk);
> + release_sock(sk);
> +
> + ret = 0;
> +
> +out:
> +#endif
> +
> + return ret;
> +}
> +
> /*
> * This is meant for all protocols to use and covers goings on
> * at the socket level. Everything here is generic.
> @@ -390,6 +445,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
> }
> #endif
>
> + if (optname == SO_BINDTODEVICE)
> + return sock_bindtodevice(sk, optval, optlen);
> +
> if (optlen < sizeof(int))
> return -EINVAL;
>
> @@ -578,54 +636,6 @@ set_rcvbuf:
> ret = sock_set_timeout(&sk->sk_sndtimeo, optval, optlen);
> break;
>
> -#ifdef CONFIG_NETDEVICES
> - case SO_BINDTODEVICE:
> - {
> - char devname[IFNAMSIZ];
> -
> - /* Sorry... */
> - if (!capable(CAP_NET_RAW)) {
> - ret = -EPERM;
> - break;
> - }
> -
> - /* Bind this socket to a particular device like "eth0",
> - * as specified in the passed interface name. If the
> - * name is "" or the option length is zero the socket
> - * is not bound.
> - */
> -
> - if (!valbool) {
> - sk->sk_bound_dev_if = 0;
> - } else {
> - if (optlen > IFNAMSIZ - 1)
> - optlen = IFNAMSIZ - 1;
> - memset(devname, 0, sizeof(devname));
> - if (copy_from_user(devname, optval, optlen)) {
> - ret = -EFAULT;
> - break;
> - }
> -
> - /* Remove any cached route for this socket. */
> - sk_dst_reset(sk);
> -
> - if (devname[0] == '\0') {
> - sk->sk_bound_dev_if = 0;
> - } else {
> - struct net_device *dev = dev_get_by_name(devname);
> - if (!dev) {
> - ret = -ENODEV;
> - break;
> - }
> - sk->sk_bound_dev_if = dev->ifindex;
> - dev_put(dev);
> - }
> - }
> - break;
> - }
> -#endif
> -
> -
> case SO_ATTACH_FILTER:
> ret = -EINVAL;
> if (optlen == sizeof(struct sock_fprog)) {
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Ben Greear <greearb@...delatech.com>
Candela Technologies Inc http://www.candelatech.com
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists