cloudius-systems / osv

OSv, a new operating system for the cloud.
osv.io
Other
4.12k stars 605 forks source link

Buggy _IOW, _IOR and _IORW ioctl macros #1313

Closed wkozaczuk closed 5 months ago

wkozaczuk commented 5 months ago

While working on adding minimal support for block device ioctl() like BLKGETSIZE64 and BLKFLSBUF, I noticed that the former one, based on the _IOR macro and defined in include/api/sys/mount.h like so:

#define BLKGETSIZE64 _IOR(0x12,114,size_t)

does not match the value of the command parameter of ioctl called by the fio program. The one expected by OSv kernel was negative in this case and after some digging, I have discovered there is a bug in the original musl code located in include/api/x64/bits/ioctl.h and include/api/aarch64/bits/ioctl.h that was fixed on musl side in this patch from 2013, in essence replacing 1, 2 and 3 with 1U, 2U, and 3U.

So this patch on OSv side fixes my issue:

diff --git a/include/api/x64/bits/ioctl.h b/include/api/x64/bits/ioctl.h
index 9a13a0eb..2d054ae9 100644
--- a/include/api/x64/bits/ioctl.h
+++ b/include/api/x64/bits/ioctl.h
@@ -4,9 +4,9 @@
 #define _IOC_READ  2U

 #define _IO(a,b) _IOC(_IOC_NONE,(a),(b),0)
-#define _IOW(a,b,c) _IOC(1,(a),(b),sizeof(c))
-#define _IOR(a,b,c) _IOC(2,(a),(b),sizeof(c))
-#define _IOWR(a,b,c) _IOC(3,(a),(b),sizeof(c))
+#define _IOW(a,b,c) _IOC(_IOC_WRITE,(a),(b),sizeof(c))
+#define _IOR(a,b,c) _IOC(_IOC_READ,(a),(b),sizeof(c))
+#define _IOWR(a,b,c) _IOC(_IOC_READ | _IOC_WRITE,(a),(b),sizeof(c))

 #define TCGETS         0x5401
 #define TCSETS         0x5402

Now there is a potential side effect of this fix, as the ioctl constants that depend on the _IOR and _IORW (the _IOW does not cause overflow) may potentially break some things in the code where they are used.

Renaming each of the 2 macros to see where compiler errors out, points to the following IOCTL macros and source code that may be affected:

#define FIONWRITE       _IOR('f', 119, int)     /* get # bytes (yet) to write */
#define FIONSPACE       _IOR('f', 118, int)     /* get space in send queue */

in bsd/sys/kern/sys_socket.cc

and

#define OSIOCGIFCONF    _IOWR('i', 20, struct bsd_ifconf)       /* get ifnet list */
#define SIOCGIFCAP      _IOWR('i', 31, struct bsd_ifreq)        /* get IF features */
#define SIOCGIFDESCR    _IOWR('i', 42, struct bsd_ifreq)        /* get ifnet descr */
#define SIOCGIFFIB      _IOWR('i', 92, struct bsd_ifreq)        /* get IF fib */
#define SIOCGIFGENERIC  _IOWR('i', 58, struct bsd_ifreq)        /* generic IF get op */
#define SIOCGIFGMEMB    _IOWR('i', 138, struct ifgroupreq) /* get members */
#define SIOCGIFGROUP    _IOWR('i', 136, struct ifgroupreq) /* get ifgroups */
#define SIOCGIFMEDIA    _IOWR('i', 56, struct ifmediareq) /* get net media */
#define SIOCGIFPDSTADDR _IOWR('i', 72, struct bsd_ifreq)        /* get gif pdst addr */
#define SIOCGIFPHYS     _IOWR('i', 53, struct bsd_ifreq)        /* get IF wire */
#define SIOCGIFPSRCADDR _IOWR('i', 71, struct bsd_ifreq)        /* get gif psrc addr */
#define SIOCGIFSTATUS   _IOWR('i', 59, struct ifstat)   /* get IF status */
#define SIOCGLIFADDR    _IOWR('i', 28, struct if_laddrreq) /* get IF addr */
#define SIOCGLIFPHYADDR _IOWR('i', 75, struct if_laddrreq) /* get gif addrs */
#define SIOCIFCREATE2   _IOWR('i', 124, struct bsd_ifreq)       /* create clone if */
#define SIOCIFCREATE    _IOWR('i', 122, struct bsd_ifreq)       /* create clone if */
#define SIOCIFGCLONERS  _IOWR('i', 120, struct if_clonereq) /* get cloners */
#define SIOCSIFMEDIA    _IOWR('i', 55, struct bsd_ifreq)        /* set net media */

in

bsd/sys/dev/xen/netfront/netfront.cc:1931:10: note: in expansion of macro ‘SIOCSIFMEDIA’
bsd/sys/dev/xen/netfront/netfront.cc:1932:10: note: in expansion of macro ‘SIOCGIFMEDIA’
bsd/sys/net/if.cc:1804:14: note: in expansion of macro ‘SIOCGIFCAP’
bsd/sys/net/if.cc:1823:14: note: in expansion of macro ‘SIOCGIFPHYS’
bsd/sys/net/if.cc:1827:14: note: in expansion of macro ‘SIOCGIFDESCR’
bsd/sys/net/if.cc:1880:14: note: in expansion of macro ‘SIOCGIFFIB’
bsd/sys/net/if.cc:2104:14: note: in expansion of macro ‘SIOCSIFMEDIA’
bsd/sys/net/if.cc:2116:14: note: in expansion of macro ‘SIOCGIFSTATUS’
bsd/sys/net/if.cc:2120:14: note: in expansion of macro ‘SIOCGIFPSRCADDR’
bsd/sys/net/if.cc:2121:14: note: in expansion of macro ‘SIOCGIFPDSTADDR’
bsd/sys/net/if.cc:2122:14: note: in expansion of macro ‘SIOCGLIFPHYADDR’
bsd/sys/net/if.cc:2123:14: note: in expansion of macro ‘SIOCGIFMEDIA’
bsd/sys/net/if.cc:2124:14: note: in expansion of macro ‘SIOCGIFGENERIC’
bsd/sys/net/if.cc:2151:14: note: in expansion of macro ‘SIOCGIFGROUP’
bsd/sys/net/if.cc:2200:14: note: in expansion of macro ‘OSIOCGIFCONF’
bsd/sys/net/if.cc:2226:14: note: in expansion of macro ‘SIOCIFCREATE’
bsd/sys/net/if.cc:2227:14: note: in expansion of macro ‘SIOCIFCREATE2’
bsd/sys/net/if.cc:2232:36: note: in expansion of macro ‘SIOCIFCREATE2’
bsd/sys/net/if.cc:2242:14: note: in expansion of macro ‘SIOCIFGCLONERS’
bsd/sys/net/if.cc:2246:14: note: in expansion of macro ‘SIOCGIFGMEMB’
bsd/sys/netinet/in.cc:270:14: note: in expansion of macro ‘SIOCGLIFADDR’
bsd/sys/netinet/in.cc:676:14: note: in expansion of macro ‘SIOCGLIFADDR’
bsd/sys/netinet/in.cc:728:14: note: in expansion of macro ‘SIOCGLIFADDR’
bsd/sys/netinet/in.cc:752:36: note: in expansion of macro ‘SIOCGLIFADDR’
bsd/sys/netinet/in.cc:777:28: note: in expansion of macro ‘SIOCGLIFADDR’

I think that possibly all of those are BSD-specific and its change should not cause any havoc.