From: Al Viro <viro@zeniv.linux.org.uk>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: netdev@vger.kernel.org, Robert O'Callahan <robert@ocallahan.org>,
Johannes Berg <johannes.berg@intel.com>
Subject: Re: [PATCH net 2/4] Revert "kill dev_ifsioc()"
Date: Sat, 26 Jan 2019 17:29:26 +0000 [thread overview]
Message-ID: <20190126172926.GG2217@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20190125214320.17685-3-johannes@sipsolutions.net>
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.
next prev parent reply other threads:[~2019-01-26 17:29 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-25 21:43 [PATCH net 0/4] various compat ioctl fixes Johannes Berg
2019-01-25 21:43 ` [PATCH net 1/4] Revert "socket: fix struct ifreq size in compat ioctl" Johannes Berg
2019-01-25 21:43 ` [PATCH net 2/4] Revert "kill dev_ifsioc()" Johannes Berg
2019-01-26 17:29 ` Al Viro [this message]
2019-01-26 17:45 ` Johannes Berg
2019-01-26 17:49 ` Johannes Berg
2019-01-26 18:53 ` Johannes Berg
2019-01-25 21:43 ` [PATCH net 3/4] net: socket: fix SIOCGIFNAME in compat Johannes Berg
2019-01-25 21:43 ` [PATCH net 4/4] net: socket: make bond ioctls go through compat_ifreq_ioctl() Johannes Berg
2019-01-28 19:22 ` [PATCH net 0/4] various compat ioctl fixes David Miller
2019-01-28 21:32 ` Johannes Berg
2019-01-30 6:19 ` David Miller
2019-01-30 15:40 ` Al Viro
2019-01-30 18:20 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190126172926.GG2217@ZenIV.linux.org.uk \
--to=viro@zeniv.linux.org.uk \
--cc=johannes.berg@intel.com \
--cc=johannes@sipsolutions.net \
--cc=netdev@vger.kernel.org \
--cc=robert@ocallahan.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).