incorrect use of sizeof() in ioctl definitions
Subsystems (5)
PATCH 1/1 incorrect use of sizeof() in ioctl definitions
Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the sizeof() operator twice. For eg:
-#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*))
+#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)
from include/asm-ia64/ioctl.h (other archs are similar):
#define _IOR(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size))
While this is not a problem for native 32 bit or native 64 platforms, it is a problem for 32 bit applications running on 64 bit platforms in compatibility mode because
sizeof(sizeof(__u32*)) on a 64 bit kernel != sizeof(sizeof(__u32*)) on a 32 bit app.
The attached patch attempts to fix the instances we could find.
-Arun
include/linux/matroxfb.h +6 -6
@@ -15,21 +15,21 @@#define MATROXFB_OUTPUT_MODE_NTSC 0x0002#define MATROXFB_OUTPUT_MODE_MONITOR 0x0080 }; -#define MATROXFB_SET_OUTPUT_MODE _IOW('n',0xFA,sizeof(struct matroxioc_output_mode)) -#define MATROXFB_GET_OUTPUT_MODE _IOWR('n',0xFA,sizeof(struct matroxioc_output_mode)) +#define MATROXFB_SET_OUTPUT_MODE _IOW('n',0xFA,struct matroxioc_output_mode) +#define MATROXFB_GET_OUTPUT_MODE _IOWR('n',0xFA,struct matroxioc_output_mode)/* bitfield */ #define MATROXFB_OUTPUT_CONN_PRIMARY (1 << MATROXFB_OUTPUT_PRIMARY)#define MATROXFB_OUTPUT_CONN_SECONDARY (1 << MATROXFB_OUTPUT_SECONDARY)#define MATROXFB_OUTPUT_CONN_DFP (1 << MATROXFB_OUTPUT_DFP)/* connect these outputs to this framebuffer */ -#define MATROXFB_SET_OUTPUT_CONNECTION _IOW('n',0xF8,sizeof(__u32)) +#define MATROXFB_SET_OUTPUT_CONNECTION _IOW('n',0xF8,__u32)/* which outputs are connected to this framebuffer */ -#define MATROXFB_GET_OUTPUT_CONNECTION _IOR('n',0xF8,sizeof(__u32)) +#define MATROXFB_GET_OUTPUT_CONNECTION _IOR('n',0xF8,__u32)/* which outputs are available for this framebuffer */ -#define MATROXFB_GET_AVAILABLE_OUTPUTS _IOR('n',0xF9,sizeof(__u32)) +#define MATROXFB_GET_AVAILABLE_OUTPUTS _IOR('n',0xF9,__u32)/* which outputs exist on this framebuffer */ -#define MATROXFB_GET_ALL_OUTPUTS _IOR('n',0xFB,sizeof(__u32)) +#define MATROXFB_GET_ALL_OUTPUTS _IOR('n',0xFB,__u32)enum matroxfb_ctrl_id {MATROXFB_CID_TESTOUT =V4L2_CID_PRIVATE_BASE ,
drivers/video/aty/aty128fb.c +2 -2
@@ -2041,9 +2041,9 @@#define ATY_MIRROR_CRT_ON 0x00000002/* out param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)/* in param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, sizeof(__u32*)) +#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, __u32*)static int aty128fb_ioctl (struct inode *inode,struct file *file,u_int cmd,u_long arg,struct fb_info *info)
include/linux/coda.h +1 -1
@@ -324,7 +324,7 @@#define VC_MAXMSGSIZE sizeof(union inputArgs)+sizeof(union outputArgs) +\ VC_MAXDATASIZE -#define CIOC_KERNEL_VERSION _IOWR('c', 10, sizeof (int)) +#define CIOC_KERNEL_VERSION _IOWR('c', 10, int)#if 0 #define CODA_KERNEL_VERSION 0/* don't care about kernel version number */
drivers/net/wan/cosa.h +6 -6
@@ -73,19 +73,19 @@#define COSAIORSET _IO('C',0xf0)/* Start microcode at given address */ -#define COSAIOSTRT _IOW('C',0xf1,sizeof(int)) +#define COSAIOSTRT _IOW('C',0xf1,int)/* Read the block from the device memory */ -#define COSAIORMEM _IOR('C',0xf2,sizeof(struct cosa_download *)) +#define COSAIORMEM _IOR('C',0xf2,struct cosa_download*)/* Write the block to the device memory (i.e. download the microcode) */ -#define COSAIODOWNLD _IOW('C',0xf2,sizeof(struct cosa_download *)) +#define COSAIODOWNLD _IOW('C',0xf2,struct cosa_download*)/* Read the device type (one of "srp", "cosa", and "cosa8" for now) */ -#define COSAIORTYPE _IOR('C',0xf3,sizeof(char *)) +#define COSAIORTYPE _IOR('C',0xf3,char*)/* Read the device identification string */ -#define COSAIORIDSTR _IOR('C',0xf4,sizeof(char *)) +#define COSAIORIDSTR _IOR('C',0xf4,char*)/* Maximum length of the identification string. */ #define COSA_MAX_ID_STRING 128
@@ -100,7 +100,7 @@#define COSAIONRCHANS _IO('C',0xf8)/* Set the driver for the bus-master operations */ -#define COSAIOBMSET _IOW('C', 0xf9, sizeof(unsigned short)) +#define COSAIOBMSET _IOW('C', 0xf9, unsigned short)#define COSA_BM_OFF 0/* Bus-mastering off - use ISA DMA (default) */ #define COSA_BM_ON 1/* Bus-mastering on - faster but untested */
include/linux/fs.h +5 -5
@@ -187,15 +187,15 @@#define BLKSSZGET _IO(0x12,104)/* get block device sector size */ #if 0 #define BLKPG _IO(0x12,105)/* See blkpg.h */ -#define BLKELVGET _IOR(0x12,106,sizeof(blkelv_ioctl_arg_t))/* elevator get */ -#define BLKELVSET _IOW(0x12,107,sizeof(blkelv_ioctl_arg_t))/* elevator set */ +#define BLKELVGET _IOR(0x12,106,blkelv_ioctl_arg_t)/* elevator get */ +#define BLKELVSET _IOW(0x12,107,blkelv_ioctl_arg_t)/* elevator set */ /* This was here just to show that the number is taken - probably all these _IO(0x12,*) ioctls should be moved to blkpg.h. */ #endif /* A jump here: 108-111 have been used for various private purposes. */ -#define BLKBSZGET _IOR(0x12,112,sizeof(int)) -#define BLKBSZSET _IOW(0x12,113,sizeof(int)) -#define BLKGETSIZE64 _IOR(0x12,114,sizeof(u64))/* return device size in bytes (u64 *arg) */ +#define BLKBSZGET _IOR(0x12,112,int) +#define BLKBSZSET _IOW(0x12,113,int) +#define BLKGETSIZE64 _IOR(0x12,114,u64)/* return device size in bytes (u64 *arg) */ #define BMAP_IOCTL 1/* obsolete - kept for compatibility */ #define FIBMAP _IO(0x00,1)/* bmap access */
include/linux/i8k.h +5 -5
@@ -22,11 +22,11 @@#define I8K_BIOS_VERSION _IOR ('i', 0x80, 4)#define I8K_MACHINE_ID _IOR ('i', 0x81, 16) -#define I8K_POWER_STATUS _IOR ('i', 0x82, sizeof(int)) -#define I8K_FN_STATUS _IOR ('i', 0x83, sizeof(int)) -#define I8K_GET_TEMP _IOR ('i', 0x84, sizeof(int)) -#define I8K_GET_SPEED _IOWR('i', 0x85, sizeof(int)) -#define I8K_GET_FAN _IOWR('i', 0x86, sizeof(int)) +#define I8K_POWER_STATUS _IOR ('i', 0x82, int) +#define I8K_FN_STATUS _IOR ('i', 0x83, int) +#define I8K_GET_TEMP _IOR ('i', 0x84, int) +#define I8K_GET_SPEED _IOWR('i', 0x85, int) +#define I8K_GET_FAN _IOWR('i', 0x86, int)#define I8K_SET_FAN _IOWR('i', 0x87, sizeof(int)*2)#define I8K_FAN_LEFT 1
include/linux/if_pppox.h +2 -2
@@ -67,9 +67,9 @@* ********************************************************************/ -#define PPPOEIOCSFWD _IOW(0xB1 ,0, sizeof(struct sockaddr_pppox)) +#define PPPOEIOCSFWD _IOW(0xB1 ,0, struct sockaddr_pppox)#define PPPOEIOCDFWD _IO(0xB1 ,1) -/*#define PPPOEIOCGFWD _IOWR(0xB1,2, sizeof(struct sockaddr_pppox))*/ +/*#define PPPOEIOCGFWD _IOWR(0xB1,2, struct sockaddr_pppox)*/ /* Codes to identify message types */ #define PADI_CODE 0x09
include/linux/pmu.h +5 -5
@@ -107,15 +107,15 @@/* no param */ #define PMU_IOC_SLEEP _IO('B', 0)/* out param: u32* backlight value: 0 to 15 */ -#define PMU_IOC_GET_BACKLIGHT _IOR('B', 1, sizeof(__u32*)) +#define PMU_IOC_GET_BACKLIGHT _IOR('B', 1, __u32*)/* in param: u32 backlight value: 0 to 15 */ -#define PMU_IOC_SET_BACKLIGHT _IOW('B', 2, sizeof(__u32)) +#define PMU_IOC_SET_BACKLIGHT _IOW('B', 2, __u32)/* out param: u32* PMU model */ -#define PMU_IOC_GET_MODEL _IOR('B', 3, sizeof(__u32*)) +#define PMU_IOC_GET_MODEL _IOR('B', 3, __u32*)/* out param: u32* has_adb: 0 or 1 */ -#define PMU_IOC_HAS_ADB _IOR('B', 4, sizeof(__u32*)) +#define PMU_IOC_HAS_ADB _IOR('B', 4, __u32*)/* out param: u32* can_sleep: 0 or 1 */ -#define PMU_IOC_CAN_SLEEP _IOR('B', 5, sizeof(__u32*)) +#define PMU_IOC_CAN_SLEEP _IOR('B', 5, __u32*)/* no param */ #define PMU_IOC_GRAB_BACKLIGHT _IOR('B', 6, 0)
include/linux/radeonfb.h +2 -2
@@ -8,8 +8,8 @@#define ATY_RADEON_CRT_ON 0x00000002 -#define FBIO_RADEON_GET_MIRROR _IOR('@', 3, sizeof(__u32*)) -#define FBIO_RADEON_SET_MIRROR _IOW('@', 4, sizeof(__u32*)) +#define FBIO_RADEON_GET_MIRROR _IOR('@', 3, __u32*) +#define FBIO_RADEON_SET_MIRROR _IOW('@', 4, __u32*)#endif
include/video/sisfb.h +2 -2
@@ -145,9 +145,9 @@/* If changing this, vgatypes.h must also be changed (for X driver) */ /* TW: ioctl for identifying and giving some info (esp. memory heap start) */ -#define SISFB_GET_INFO _IOR('n',0xF8,sizeof(__u32)) +#define SISFB_GET_INFO _IOR('n',0xF8,__u32) -#define SISFB_GET_VBRSTATUS _IOR('n',0xF9,sizeof(__u32)) +#define SISFB_GET_VBRSTATUS _IOR('n',0xF9,__u32)/* TW: Structure argument for SISFB_GET_INFO ioctl */ typedef struct _SISFB_INFO sisfb_info , *psisfb_info ;
Quoted 9 lines · Arun Sharma
Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the
sizeof()operator twice. For eg:(quoted diff) +1
-1-#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)from
include/asm-ia64/ioctl.h(other archs are similar):#define
_IOR(type,nr,size)_IOC(_IOC_READ,(type),(nr),sizeof(size))
Matthew Wilcox fixed all except one of these a while back. What is left over is this chunk:
drivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +2 -2
@@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re#define ATY_MIRROR_CRT_ON 0x00000002/* out param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)/* in param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, sizeof(__u32*)) +#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, __u32*)static int aty128fb_ioctl (struct inode *inode,struct file *file,u_int cmd,u_long arg,struct fb_info *info)
Matthew's conversion mainly converted things to size_t, but from the looks of it, __u32* is the right thing to use in this case, I think?
Quoted 17 lines · Andrew Morton
diff -puN
drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fixdrivers/video/aty/aty128fb.cdrivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +2
-2@@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re#define ATY_MIRROR_CRT_ON 0x00000002/* out param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)/* in param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, sizeof(__u32*)) +#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, __u32*)static int aty128fb_ioctl (struct inode *inode,struct file *file,u_int cmd,u_long arg,struct fb_info *info)
Quoted 2 lines
Matthew's conversion mainly converted things to
size_t, but from the looks of it, __u32* is the right thing to use in this case, I think?
sizeof(__u32*) may not be sizeof(sizeof(__u32*)), so this would be an API change... Therefore, all these wrong entries need to change to size_t (preferably with the real type following inside a comment so we don't loose useful information.)
Signature · 4 lines
Russell King ([email protected]) http://www.arm.linux.org.uk/personal/ Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/ 2.6 Serial core
Quoted 12 lines · Russell King
Quoted 12 lines
/* out param: u32* backlight value: 0 to 15 */
drivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +2
-2-#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)/* in param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, sizeof(__u32*)) +#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, __u32*)static int aty128fb_ioctl (struct inode *inode,struct file *file,u_int cmd,u_long arg,struct fb_info *info)
Quoted 7 lines
Quoted 2 lines
Matthew's conversion mainly converted things to
size_t, but from the looks of it, __u32* is the right thing to use in this case, I think?
sizeof(__u32*)may not be sizeof(sizeof(__u32*)), so this would be an API change... Therefore, all these wrong entries need to change tosize_t(preferably with the real type following inside a comment so we don't loose useful information.)
In that case I'm going to need a bit of education as to what the whole thing is trying to do.
If FBIO_ATY128_SET_MIRROR is really supposed to be passing a pointer into the ioctl then we *want* the encoded ioctl number to be different for 32-bit-compiled userspace and 64-bit-compiled userspace, don't we?
Quoted 6 lines · Andrew Morton
drivers/video/aty/aty128fb.c aty128fb_setcolreg(u_int regno, u_int re · line 2044
Quoted 5 lines · Arun Sharma
Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the
sizeof()operator twice. For eg:drivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +1
-1-#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)
The real problem is that ia64 doesn't have the typechecking ioctl definitions yet. Here's what I committed for parisc recently (it applies to ia64 too):
+++ include/asm-parisc/ioctl.h 21 Sep 2003 01:01:38 -0000 1.2
@@ -44,11 +44,21 @@
((nr) << _IOC_NRSHIFT) | \
((size) << _IOC_SIZESHIFT))+/* provoke compile error for invalid uses of size argument */ +extern int __invalid_size_argument_for_IOC; +#define _IOC_TYPECHECK(t) \
+ ((sizeof(t) == sizeof(t[1]) && \
+ sizeof(t) < (1 << _IOC_SIZEBITS)) ? \
+ sizeof(t) : __invalid_size_argument_for_IOC)
+
/* used to create numbers */
#define _IO(type,nr) _IOC(_IOC_NONE,(type),(nr),0) -#define _IOR(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size)) -#define _IOW(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),sizeof(size)) -#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),sizeof(siz
e))
+#define _IOR(type,nr,size) _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)
))
+#define _IOW(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size
)))
+#define _IOWR(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPE
CHECK(size)))
+#define _IOR_BAD(type,nr,size) _IOC(_IOC_READ,(type),(nr),sizeof(size)) +#define _IOW_BAD(type,nr,size) _IOC(_IOC_WRITE,(type),(nr),sizeof(size))
+#define _IOWR_BAD(type,nr,size) _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),si
zeof(size))
/* used to decode ioctl numbers.. */
#define _IOC_DIR(nr) (((nr) >> _IOC_DIRSHIFT) & _IOC_DIRMASK)
Quoted 21 lines
Matthew Wilcox fixed all except one of these a while back. What is left over is this chunk:
diff -puN
drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fixdrivers/video/aty/aty128fb.cdrivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +2
-2@@ -2041,9 +2041,9 @@ aty128fb_setcolreg(u_int regno, u_int re#define ATY_MIRROR_CRT_ON 0x00000002/* out param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)/* in param: u32* backlight value: 0 to 15 */ -#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, sizeof(__u32*)) +#define FBIO_ATY128_SET_MIRROR _IOW('@', 2, __u32*)static int aty128fb_ioctl (struct inode *inode,struct file *file,u_int cmd,u_long arg,struct fb_info *info)
Quoted 2 lines
Matthew's conversion mainly converted things to
size_t, but from the looks of it, __u32* is the right thing to use in this case, I think?
I think so, given:
radeonfb.h:#define FBIO_RADEON_GET_MIRROR _IOR('@', 3, size_t)
radeonfb.h:#define FBIO_RADEON_SET_MIRROR _IOW('@', 4, size_t)
Though we can also change it to _IOR_BAD and _IOW_BAD.
Signature · 3 lines
"It's not Hollywood. War is real, war is primarily not about defeat or victory, it is about death. I've seen thousands and thousands of dead bodies. Do you think I want to have an academic debate on this subject?" -- Robert Fisk
Quoted 7 lines
Quoted 2 lines
Matthew's conversion mainly converted things to
size_t, but from the looks of it, __u32* is the right thing to use in this case, I think?
sizeof(__u32*)may not be sizeof(sizeof(__u32*)), so this would be an API change... Therefore, all these wrong entries need to change tosize_t(preferably with the real type following inside a comment so we don't loose useful information.)
I hit this bug a while back - most often it's for 4byte sized objects (ints or pointers) on x86 arch (don't know about others). Unfortunately there was a case where it was for an 8 and was wrongly declared via sizeof and thus 4. I'd suggest moving all to single sizeof's and where this causes API changes to implement bogus/extra declarations which could then possibly be faded out with time. However currently the glibc kernel headers are also similarly screwed. Obviously this would bloat the kernel a little, OTOH for x86 arch I only noticed one case where it didn't end up the same whether thru double or single sizeof. Perhaps we could be lucky and have almost as much luck on other archs?
MaZe.
Quoted 8 lines · Matthew Wilcox
Quoted 7 lines · Andrew Morton
drivers/video/aty/aty128fb.c aty128fb_setcolreg(u_int regno, u_int re · line 2044
Quoted 6 lines · Arun Sharma
Some drivers seem to use macros such as _IOR/_IOW in a way that ends up calling the
sizeof()operator twice. For eg:drivers/video/aty/aty128fb.c renamed from drivers/video/aty/aty128fb.c~sizeof-in-ioctl-fix +1
-1-#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*)
But this changes the define. You want
#define FBIO_ATY128_GET_MIRROR _IOR_BAD('@', 1, __u32*)
Quoted 3 lines
+#define
_IOR(type,nr,size)_IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)
))
+#define_IOR_BAD(type,nr,size)_IOC(_IOC_READ,(type),(nr),sizeof(size))
Something else we should do is to change all occurrences of 'size' here into 'argtype'. All this nonsense came because of the bad choice of identifier.