On Fri, Jan 25, 2019 at 10:43:18PM +0100, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> 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.
