EVMS core 3/4: evms_ioctl.h
Review
Quoted 6 lines · Kevin Corry
(quoted diff) +6
-0+struct evms_plugin_ioctl_pkt { + ulong feature_id; + s32 feature_command; + s32 status; + void *feature_ioctl_data; +};
This is passed between user space and kernel space right?
For 32bit emulation on 64bit purposes you should always use explicitely sized types (u32/u64 not ulong). The pointer will still need to be converted. Best is to avoid pointers if possible (e.g. couldn't the data just be tacked on here?)
Review
Quoted 13 lines
(quoted diff) +13
-0+#define EVMS_EVENT_END_OF_DISCOVERY 0 + +/** + * struct evms_notify_pkt - evms event notification ioctl packet definition + * @command: 0 = unregister, 1 = register + * @eventry: event structure + * @status: returned operation status + * + * ioctl packet definition for EVMS_PROCESS_NOTIFY_EVENT ioctl + **/ +struct evms_notify_pkt { + s32 command; + struct evms_event eventry;
If eventry contains any potential 64bit stuff it would be best to align it to 64bit explicitely
Review
Quoted 9 lines
+ **/
(quoted diff) +8
-0+struct evms_user_disk_info_pkt { + u32 status; + u32 flags; + u64 disk_handle; + u32 disk_dev; + u32 geo_sectors; + u32 geo_heads; + u64 geo_cylinders;
emulation trap: on x86-64/ia64 u64 have different alignment on 32bit vs 64bit (4 bytes vs natural). Please make sure that u64 is always explicitely 64bit aligned. It isn't here.
Review
Quoted 3 lines
(quoted diff) +3
-0+ u64 disk_handle; + s32 io_flag; + u64 starting_sector;
Same issue
It would be best to clean the ABI up now when you can still change it. Otherwise the emulation functions later will be very ugly (take a look at the LVM horror in arch/x86_64/ia32/ia32_ioctl.c for a bad example - LVM1 wasn't cleaned up in time)
-Andi
Quoted 10 lines · Andi Kleen
Review
Quoted 6 lines · Kevin Corry
(quoted diff) +6
-0+struct evms_plugin_ioctl_pkt { + ulong feature_id; + s32 feature_command; + s32 status; + void *feature_ioctl_data; +};This is passed between user space and kernel space right?
Quoted 4 lines
For 32bit emulation on 64bit purposes you should always use explicitely sized types (u32/u64 not ulong). The pointer will still need to be converted. Best is to avoid pointers if possible (e.g. couldn't the data just be tacked on here?)
The ulong is definitely wrong. Should be u32.
In general, we are aware of the issues with using 32-bit user-space on top of 64-bit kernel. If you take a look at evms.c you will find several functions that get registered at init-time with the 32-to-64-bit ioctl conversion code. These take care of translating pointers from user-space to kernel-space in this situation. EVMS has been tested on ppc64 with success, and we have someone currently running tests on sparc64 to make sure it works there as well.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +4
-0+#define EVMS_EVENT_END_OF_DISCOVERY 0 + +/** + * struct evms_notify_pkt - evms event notification ioctl packetdefinition + * @command: 0 = unregister, 1 = register
+ * @eventry: event structure
+ * @status: returned operation status
+ *
Quoted 8 lines
Review
Quoted 5 lines
+ * ioctl packet definition for
EVMS_PROCESS_NOTIFY_EVENTioctl
+ **/(quoted diff) +3
-0+struct evms_notify_pkt { + s32 command; + struct evms_event eventry;If eventry contains any potential 64bit stuff it would be best to align it to 64bit explicitely
Correct. In this particular case we are safe, since struct evms_event only contains 32bit fields. But we may switch this around anyway just to be even safer.
Quoted 14 lines
Review
Quoted 9 lines
+ **/
(quoted diff) +8
-0+struct evms_user_disk_info_pkt { + u32 status; + u32 flags; + u64 disk_handle; + u32 disk_dev; + u32 geo_sectors; + u32 geo_heads; + u64 geo_cylinders;emulation trap: on x86-64/ia64 u64 have different alignment on 32bit vs 64bit (4 bytes vs natural). Please make sure that u64 is always explicitely 64bit aligned. It isn't here.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ u64 disk_handle; + s32 io_flag; + u64 starting_sector;Same issue
Yep. We'll get those fixed. Thanks for pointing these out.
Quoted 7 lines
It would be best to clean the ABI up now when you can still change it. Otherwise the emulation functions later will be very ugly (take a look at the LVM horror in
arch/x86_64/ia32/ia32_ioctl.cfor a bad example - LVM1 wasn't cleaned up in time)-Andi
Thanks for the suggestions!
Signature · 3 lines
Kevin Corry [email protected] http://evms.sourceforge.net/
Quoted 7 lines · Kevin Corry
In general, we are aware of the issues with using 32-bit user-space on top of 64-bit kernel. If you take a look at
evms.cyou will find several functions that get registered at init-time with the 32-to-64-bit ioctl conversion code. These take care of translating pointers from user-space to kernel-space in this situation. EVMS has been tested on ppc64 with success, and we have someone currently running tests on sparc64 to make sure it works there as well.
I think you have some security holes in there:
+parms.buffer_address = (u8 *)uvirt_to_kernel(parms32.buffer_address); [...] +set_fs(KERNEL_DS); +rc = sys_ioctl(fd, kcmd, (unsigned long)karg); +set_fs(old_fs);
parms32.buffer_address comes from user space. With the set_fs you turn off all access checking. Surely when whatever sits at the bottom of sys_ioctl accesses it it'll use copy_from/to_user and it will do an unchecked reference of a user supplied pointer, allowing it to read/write all memory.
Same bug is present in more functions.
The rule is: when you do set_fs(KERNEL_DS) you have to copy all user supplied pointers before it.
-Andi