netdev - Re: [PATCH net 2/4] Revert "kill dev

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 26 Jan 2019 17:29:26 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, Robert O'Callahan <robert@...llahan.org>,
        Johannes Berg <johannes.berg@...el.com>
Subject: Re: [PATCH net 2/4] Revert "kill dev_ifsioc()"

On Fri, Jan 25, 2019 at 10:43:18PM +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.com>
> 
> This reverts commit bf4405737f9f ("kill dev_ifsioc()").
> 
> This wasn't really unused as implied by the original commit,
> it still handles the copy to/from user differently, and the
> commit thus caused issues such as
>   https://bugzilla.kernel.org/show_bug.cgi?id=199469
> and
>   https://bugzilla.kernel.org/show_bug.cgi?id=202273
> 
> However, deviating from a strict revert, rename dev_ifsioc()
> to compat_ifreq_ioctl() to be clearer as to its purpose and
> add a comment.

I disagree with solution.  Look at what's happening here:

> +	uifr = compat_alloc_user_space(sizeof(*uifr));
> +	if (copy_in_user(uifr, uifr32, sizeof(*uifr32)))
> +		return -EFAULT;

an enlarged copy is made.

> +	err = sock_do_ioctl(net, sock, cmd, (unsigned long)uifr);

... which hits this:
                if (copy_from_user(&ifr, argp, ifreq_size))
                        return -EFAULT;
                err = dev_ioctl(net, cmd, &ifr, &need_copyout);
                if (!err && need_copyout)
                        if (copy_to_user(argp, &ifr, ifreq_size))
                                return -EFAULT;
copying that copy into the kernel space, passing _that_ to dev_ioctl(),
then, if dev_ioctl() says that this one needs a copyout, we take the
modified kernel copy and copy it to (enlarged) userland one.

Then
> +
> +	if (!err) {
> +		switch (cmd) {
> +		case SIOCGIFFLAGS:
> +		case SIOCGIFMETRIC:
> +		case SIOCGIFMTU:
> +		case SIOCGIFMEM:
> +		case SIOCGIFHWADDR:
> +		case SIOCGIFINDEX:
> +		case SIOCGIFADDR:
> +		case SIOCGIFBRDADDR:
> +		case SIOCGIFDSTADDR:
> +		case SIOCGIFNETMASK:
> +		case SIOCGIFPFLAGS:
> +		case SIOCGIFTXQLEN:
> +		case SIOCGMIIPHY:
> +		case SIOCGMIIREG:
> +			if (copy_in_user(uifr32, uifr, sizeof(*uifr32)))
> +				err = -EFAULT;

We duplicate the "needs copyout" logics here, and copy from the enlarged
userland instance to the original.

It's much too convoluted, and I really wonder if ifreq_size argument is
a good idea - AFAICS, it's only introduced to be able to (ab)use
sock_do_ioctl() here.