hotfix: bad copy from userspace by ExpressMIMO2 kernel driver

The ExpressMIMO2 kernel driver did not use proper mechanisms
to copy from user space.

It is very surprising that it ever worked at all...

This commit is a quick fix. It is not the end of the story
and some more work may be needed if things don't work.

The remaining issue is that we pass pointer addresses to the
kernel as 32 bits values, but pointers are 64 bits values
(on x86_64, our supported platform). Some checks have been
put in place to detect if upper 32 bits of a pointer are
not 0 and print a strong error message in case of problem,
but no clean solution has been implemented.

targets/ARCH/EXMIMO/DRIVER/eurecom/fileops.c

@@ -179,6 +179,8 @@ int openair_device_ioctl(struct inode *inode,struct file *filp, unsigned int cmd
 
   int tmp;
 
+  unsigned int user_args[4];
+
   static unsigned int update_firmware_command;
   static unsigned int update_firmware_address;
   static unsigned int update_firmware_length;
@@ -328,15 +330,38 @@ int openair_device_ioctl(struct inode *inode,struct file *filp, unsigned int cmd
      acknowledge with no limit */
 
 #define MAX_IOCTL_ACK_CNT    500
-    update_firmware_command = *((unsigned int*)arg);
 
+    /* 'arg' is an unsigned long and is supposed to be big enough
+     * to hold an address - this hypothesis is okay for i386 and x86_64
+     * maybe not somewhere else
+     * this will probably fail with older kernels
+     * (works with 3.17 on x86_64)
+     */
+    tmp = copy_from_user(&user_args, (void*)arg, 4*sizeof(unsigned int));
+    if (tmp) {
+      printk("[openair][IOCTL] UPDATE_FIRMWARE: error copying parameters to kernel-space (%d bytes uncopied).\n", tmp);
+      return -1;
+    }
+
+    update_firmware_command = user_args[0];
 
     switch (update_firmware_command) {
     case UPDATE_FIRMWARE_TRANSFER_BLOCK:
-      update_firmware_address   = ((unsigned int*)arg)[1];
-      update_firmware_length    = ((unsigned int*)arg)[2];
+      update_firmware_address   = user_args[1];
+      update_firmware_length    = user_args[2];
+
+      /* This is totally wrong on x86_64: a pointer is 64 bits and
+       * unsigned int is 32 bits. The userspace program has to ensure
+       * that its buffer address fits into 32 bits.
+       * If it proves a too strong requirement, we may change things
+       * in the future.
+       * The compiler emits a warning here. Do not remove this warning!
+       * This is to clearly remember this problem.
+       * If you require the compilation to work with zero warning,
+       * consider this one as an exception and find a proper workaround.
+       */
+      update_firmware_ubuffer   = user_args[3];
 
-      update_firmware_ubuffer   = (unsigned int*)((unsigned int*)arg)[3];
       update_firmware_kbuffer = (unsigned int*)kmalloc(update_firmware_length * 4 /* 4 because kmalloc expects bytes */,
                                 GFP_KERNEL);
 
@@ -390,8 +415,8 @@ int openair_device_ioctl(struct inode *inode,struct file *filp, unsigned int cmd
 
     case UPDATE_FIRMWARE_CLEAR_BSS:
 
-      update_firmware_bss_address   = ((unsigned int*)arg)[1];
-      update_firmware_bss_size      = ((unsigned int*)arg)[2];
+      update_firmware_bss_address   = user_args[1];
+      update_firmware_bss_size      = user_args[2];
       sparc_tmp_0 = update_firmware_bss_address;
       sparc_tmp_1 = update_firmware_bss_size;
 
@@ -411,8 +436,8 @@ int openair_device_ioctl(struct inode *inode,struct file *filp, unsigned int cmd
 
     case UPDATE_FIRMWARE_START_EXECUTION:
 
-      update_firmware_start_address = ((unsigned int*)arg)[1];
-      update_firmware_stack_pointer = ((unsigned int*)arg)[2];
+      update_firmware_start_address = user_args[1];
+      update_firmware_stack_pointer = user_args[2];
       sparc_tmp_0 = update_firmware_start_address;
       sparc_tmp_1 = update_firmware_stack_pointer;
 

targets/ARCH/EXMIMO/USERSPACE/OAI_FW_INIT/updatefw.c

@@ -41,6 +41,7 @@
 #include <getopt.h>
 #include <string.h>
 #include <unistd.h>
+#include <stdint.h>
 
 #include <sys/ioctl.h>
 #include "openair_device.h"
@@ -239,6 +240,18 @@ void find_and_transfer_section(char* section_name, unsigned int verboselevel) {
       /* Dynamically allocate a chunk of memory to store the section into. */
       section_content = (char*)malloc(elf_Shdr.sh_size);
 
+      /* Fail if the address returned by malloc does not fit in 32 bits.
+       * The kernel driver gets this address as 32 bits and uses it to copy
+       * from userspace. If the address does not fit into 32 bits the kernel
+       * will copy garbage or fail to copy completely.
+       * To be reworked if things do not work on some setups.
+       */
+      if (sizeof(char*) > 4 && (((uintptr_t)section_content)>>32)) {
+        fprintf(stderr, "FATAL ERROR: an internal serious problem has been encountered.\n");
+        fprintf(stderr, "Contact the authors so as to solve this issue.\n");
+        exit(-1);
+      }
+
       if (!section_content) {
         fprintf(stderr, "Error: could not dynamically allocate %d bytes for section %s.\n",
                 elf_Shdr.sh_size, SecNameStnTable + elf_Shdr.sh_name);
@@ -281,6 +294,13 @@ void find_and_transfer_section(char* section_name, unsigned int verboselevel) {
         ioctl_params[0] = UPDATE_FIRMWARE_TRANSFER_BLOCK;
         ioctl_params[1] = elf_Shdr.sh_addr;
         ioctl_params[2] = elf_Shdr.sh_size / 4;
+        /* This is wrong on x86_64 because a pointer is 64 bits and
+         * an unsigned int is only 32 bits.
+         * The compiler emits a warning, but the test
+         * above on the value of section_content makes it work
+         * (hopefully).
+         * To be changed if things do not work.
+         */
         ioctl_params[3] = (unsigned int)((unsigned int*)section_content);
         //invert4(ioctl_params[1]);
         //invert4(ioctl_params[2]);