Re: [PATCH net 2/4] Revert "kill dev_ifsioc()"

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Jan 25, 2019 at 10:43:18PM +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.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.