Bug 195869

Summary: Wireless ioctls fail if struct iwreq is at the end of a memory mapping
Product: Networking Reporter: Robert O'Callahan (robert)
Component: WirelessAssignee: Johannes Berg (johannes)
Status: CLOSED CODE_FIX    
Severity: normal    
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.12rc12 Subsystem:
Regression: No Bisected commit-id:
Attachments: complete reduced testcase
tentative fix

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.