Bug 195869 - Wireless ioctls fail if struct iwreq is at the end of a memory mapping
Summary: Wireless ioctls fail if struct iwreq is at the end of a memory mapping
Status: CLOSED CODE_FIX
Alias: None
Product: Networking
Classification: Unclassified
Component: Wireless (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Johannes Berg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-24 02:21 UTC by Robert O'Callahan
Modified: 2017-06-14 12:03 UTC (History)
0 users

See Also:
Kernel Version: 4.12rc12
Tree: Mainline
Regression: No


Attachments
complete reduced testcase (818 bytes, text/plain)
2017-05-24 02:21 UTC, Robert O'Callahan
Details
tentative fix (2.22 KB, patch)
2017-06-14 11:33 UTC, Johannes Berg
Details | Diff

Description Robert O'Callahan 2017-05-24 02:21:26 UTC
Created attachment 256697 [details]
complete reduced testcase

If you do, say
  struct iwreq* req = ...;
  int ret = ioctl(sockfd, SIOCGIWNAME, req);
the ioctl will fail with EFAULT if req points to valid memory that is exactly sizeof(struct iwreq). Full testcase attached.

The problem is that dev_ioctl() does a copy_from_user(sizeof(struct ifreq)). On x86-64 that's 40 bytes, but sizeof(struct iwreq) is only 32 bytes.

This isn't a huge deal, but it does mean that user code that stores struct iwreq on the heap can randomly fail once in a blue moon, and that's bad.

Alternatively it could be documented somewhere that user code should actually pass in a struct ifreq and cast it to struct iwreq for all usage, but that's horrible.
Comment 1 Johannes Berg 2017-06-14 06:18:45 UTC
Thanks for the report, and sorry it went unnoticed for this long.

Do you want to propose a patch, since you've already analysed the problem so deeply?
Comment 2 Johannes Berg 2017-06-14 07:41:35 UTC
It's somewhat involved actually, I'm fixing it.
Comment 3 Robert O'Callahan 2017-06-14 10:37:38 UTC
Thanks!
Comment 4 Johannes Berg 2017-06-14 11:31:04 UTC
I have a fix, want to test it? Will attach a minimal version of the fix.
Comment 5 Robert O'Callahan 2017-06-14 11:32:24 UTC
I'll test it but I doubt my testing will add anything over yours ...
Comment 6 Johannes Berg 2017-06-14 11:33:03 UTC
Created attachment 256997 [details]
tentative fix
Comment 7 Johannes Berg 2017-06-14 11:39:19 UTC
Just to give you an idea of what kind of "gem" you found here - this bug was introduced in mainline in 2.1.15, almost exactly 20.5 years (!!) ago today (released Dec 12, 1996).
Comment 8 Johannes Berg 2017-06-14 11:42:55 UTC
I'm still waiting for the kernel to finish compiling before I can test it, but I'm pretty confident :)
Comment 9 Johannes Berg 2017-06-14 11:50:49 UTC
It finished, and I can confirm the bug is fixed:

without patch:

# /tmp/test wlan0
Failed SIOCGIWNAME: Bad address


with patch:

# /tmp/test wlan0
Failed SIOCGIWNAME: No such device

yeah, I didn't even have one, but it doesn't matter :-)

with a device existing I get 

# /tmp/test wlan0
wireless protocol name: IEEE 802.11
Comment 10 Robert O'Callahan 2017-06-14 11:51:19 UTC
Great!
Comment 11 Johannes Berg 2017-06-14 11:56:15 UTC
I'll also say that you should probably stop using wireless ioctls and use nl80211 instead :-)
Comment 12 Robert O'Callahan 2017-06-14 11:57:52 UTC
OK. I found this while working on rr (http://rr-project.org/), so this is about recording the execution of applications using those ioctls, not really using them ourselves :-). Thanks again.

Note You need to log in before you can comment on or make changes to this bug.