On Sat, 2019-01-26 at 17:29 +0000, Al Viro wrote:
>
> 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.
Yes, that's the point :-)
> > + 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.
Yes and no. It *sometimes* (actually rarely, since we don't really have
dev_ioctls that much, afaict) hits this, but it could also just hit
static long sock_do_ioctl(struct net *net, struct socket *sock,
unsigned int cmd, unsigned long arg)
{
[...]
err = sock->ops->ioctl(sock, cmd, arg);
[...]
if (err != -ENOIOCTLCMD)
return err;
and *not* get to the code you quote, i.e. *not* be a dev_ioctl().
> 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.
That was the other (my) patch I reverted, it was done so we could do the
copy in/out here safely for dev_ioctl(), but it clearly doesn't work for
the "sock->ops->ioctl()" case.
If you have any better suggestions, I don't mind, but I don't see
anything short of passing compat knowledge down to sock->ops->ioctl()
which seems like an even worse idea.
johannes
