Bug 202273

Summary: Regression in 32-bit compat SIOCGIFADDR ioctl due to bf4405737f9f85a06db2b0ce5d76a818b61992e2
Product: Networking Reporter: Robert O'Callahan (robert)
Component: OtherAssignee: Stephen Hemminger (stephen)
Status: NEW ---    
Severity: normal CC: davem, johannes, viro
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.19.10-200.fc28.x86_64 Subsystem:
Regression: No Bisected commit-id:
Attachments: testcase

Description Robert O'Callahan 2019-01-15 06:19:39 UTC 
Created attachment 280477 [details]
testcase

This is very similar to bug 199469. The fix there didn't fix all the relevant ioctls.

Basically the problem is that SIOCGIFADDR is handled by inet_ioctl() via `err = sock->ops->ioctl(sock, cmd, arg);` in do_sock_ioctl(), before we get to the do_sock_ioctl() compat code added in 1cebf8f143c21eb422cd0f4e27ab2ae366eb4d04 to fix bug 199469. So SIOCGIFADDR, and I guess any other ioctl handled by sock->ops that reads/writes "struct ifreq", is broken in 32-bit compat.

Testcase attached. Output:

[roc@localhost ~]$ gcc -o ~/tmp/test ~/tmp/test.c
[roc@localhost ~]$ ~/tmp/test
[roc@localhost ~]$ gcc -m32 -o ~/tmp/test ~/tmp/test.c
[roc@localhost ~]$ ~/tmp/test
Failed SIOCGIFINDEX: Bad address
Comment 1 Johannes Berg 2019-01-15 07:38:45 UTC 
commits 1cebf8f143c21eb422cd0f4e27ab2ae366eb4d04 and bf4405737f9f85a06db2b0ce5d76a818b61992e2 seem to revert fine (in this order). Can you please test the resulting kernel with those reverted?

I think that's the best course of action at this point.
Comment 2 Robert O'Callahan 2019-01-15 19:46:21 UTC 
Those reverts fix that bug.

But they introduce a new, very similar bug with SIOCGIFNAME:

#include <linux/if.h>
#include <stdio.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/user.h>
#include <sys/socket.h>
#include <sys/types.h>

int main(void) {
  int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
  char* p = (char*)mmap(NULL, PAGE_SIZE*2, PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  munmap(p + PAGE_SIZE, PAGE_SIZE);
  struct ifreq* req = (struct ifreq*)(p + PAGE_SIZE - sizeof(struct ifreq));
  req->ifr_ifindex = 0;
  int ret = ioctl(sockfd, SIOCGIFNAME, req);
  if (ret < 0) {
    perror("Failed SIOCGIFNAME");
    return 1;
  }
  return 0;
}

This now fails with EFAULT.

Pardon me for saying so but I think the kernel really needs a decent test suite for these syscalls that runs frequently in both 64-bit and 32-bit-compat userspace. We're kind of flailing in the dark here.
Comment 3 Robert O'Callahan 2019-01-15 19:48:22 UTC 
(rr's test suite isn't designed to catch kernel bugs but apparently it's better at catching these bugs than whatever tests the kernel community currently runs.)
Comment 4 Johannes Berg 2019-01-25 20:55:25 UTC 
"This now fails with EFAULT."

Hmm, but those reverts don't touch the SIOCGIFNAME path at all?

I'll dig. And yeah, I guess the kernel has no test suite. I don't really feel all that responsible, I just happen to deal with wireless extensions ioctls once a while, but it's not really my area in any case...
Comment 5 Johannes Berg 2019-01-25 20:59:55 UTC 
Ah, no ... my patch had explicitly fixed SIOCGIFNAME... that's why