Re: [PATCH v2] net: don't unconditionally copy_from_user a struct ifreq for socket ioctls

[Peter Collingbourne <pcc@google.com>]

On Wed, Sep 1, 2021 at 7:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 1 Sep 2021 08:22:42 +0000 David Laight wrote:
> > From: Jakub Kicinski
> > > Sent: 31 August 2021 17:30
> > >
> > > On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:
> > > > @@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
> > > >   struct ifreq ifreq;
> > > >   u32 data32;
> > > >
> > > > + if (!is_socket_ioctl_cmd(cmd))
> > > > +         return -ENOTTY;
> > > >   if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
> > > >           return -EFAULT;
> > > >   if (get_user(data32, &u_ifreq32->ifr_data))
> > >
> > > Hi Peter, when resolving the net -> net-next merge conflict I couldn't
> > > figure out why this chunk is needed. It seems all callers of
> > > compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
> > > Please double check my resolution (tip of net-next) and if this is
> > > indeed unnecessary perhaps send a cleanup? Thanks!
> >
> > To stop the copy_from_user() faulting when the user buffer
> > isn't long enough.
> > In particular for iasatty() on arm with tagged pointers.
>
> Let me rephrase. is_socket_ioctl_cmd() is always true here. There were
> only two callers, both check cmd is of specific, "sockety" type.

I see, it looks like we don't need the check on the compat path then.

I can send a followup to clean this up but given that I got a comment
from another reviewer saying that we should try to make the native and
compat paths as similar as possible, maybe it isn't too bad to leave
things as is?

Peter