[PATCH v3] staging: pi433: fix race condition in pi433_ioctl
In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is modified via copy_from_user(&instance->tx_cfg, argp, sizeof(struct pi433_tx_cfg))) without any kind of synchronization. In the case where two threads would execute this same command concurrently the tx_cfg field might enter in an inconsistent state. Additionally: if ioctl(PI433_IOC_WR_TX_CFG) and write() execute concurrently the tx config might be modified while it is being copied to the fifo, resulting in potential data corruption. Fix: Get instance->tx_cfg_lock before modifying tx config in the PI433_IOC_WR_TX_CFG case in pi433_ioctl. Also, do not copy data directly from user space to instance->tx_cfg. Instead use a temporary buffer allowing future checks for correctness of copied data and simpler code. Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com> --- Changes in v3: - Use tx_cfg for the name of temporary buffer (shorter, allows copy_from_user call to fit in a single line). - Move mutex_{un,}lock calls around memcpy instead of before copy_from_user call (was useless since we use a temporary buffer). - Remove useless comment before mutex_lock. --- drivers/staging/pi433/pi433_if.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c index b061f77dda41..53a69cb37056 100644 --- a/drivers/staging/pi433/pi433_if.c +++ b/drivers/staging/pi433/pi433_if.c @@ -880,6 +880,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) int retval = 0; struct pi433_instance *instance; struct pi433_device *device; + struct pi433_tx_cfg tx_cfg; void __user *argp = (void __user *)arg; /* Check type and command number */ @@ -902,9 +903,11 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return -EFAULT; break; case PI433_IOC_WR_TX_CFG: - if (copy_from_user(&instance->tx_cfg, argp, - sizeof(struct pi433_tx_cfg))) + if (copy_from_user(&tx_cfg, argp, sizeof(struct pi433_tx_cfg))) return -EFAULT; + mutex_lock(&device->tx_fifo_lock); + memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); + mutex_unlock(&device->tx_fifo_lock); break; case PI433_IOC_RD_RX_CFG: if (copy_to_user(argp, &device->rx_cfg, -- 2.17.1
Hi Hugo, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on v4.17 next-20180613] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hugo-Lefeuvre/staging-pi433-fix-rac... config: i386-randconfig-s0-201823 (attached as .config) compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): In file included from arch/x86/include/asm/string.h:3:0, from include/linux/string.h:20, from arch/x86/include/asm/page_32.h:35, from arch/x86/include/asm/page.h:14, from arch/x86/include/asm/thread_info.h:12, from include/linux/thread_info.h:38, from arch/x86/include/asm/preempt.h:7, from include/linux/preempt.h:81, from include/linux/spinlock.h:51, from include/linux/seqlock.h:36, from include/linux/time.h:6, from include/linux/stat.h:19, from include/linux/module.h:10, from drivers/staging//pi433/pi433_if.c:31: drivers/staging//pi433/pi433_if.c: In function 'pi433_ioctl':
drivers/staging//pi433/pi433_if.c:909:30: error: 'tx_cfg_buffer' undeclared (first use in this function) memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); ^ arch/x86/include/asm/string_32.h:183:45: note: in definition of macro 'memcpy' #define memcpy(t, f, n) __builtin_memcpy(t, f, n) ^ drivers/staging//pi433/pi433_if.c:909:30: note: each undeclared identifier is reported only once for each function it appears in memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); ^ arch/x86/include/asm/string_32.h:183:45: note: in definition of macro 'memcpy' #define memcpy(t, f, n) __builtin_memcpy(t, f, n) ^
vim +/tx_cfg_buffer +909 drivers/staging//pi433/pi433_if.c 876 877 static long 878 pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) 879 { 880 int retval = 0; 881 struct pi433_instance *instance; 882 struct pi433_device *device; 883 struct pi433_tx_cfg tx_cfg; 884 void __user *argp = (void __user *)arg; 885 886 /* Check type and command number */ 887 if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC) 888 return -ENOTTY; 889 890 /* TODO? guard against device removal before, or while, 891 * we issue this ioctl. --> device_get() 892 */ 893 instance = filp->private_data; 894 device = instance->device; 895 896 if (!device) 897 return -ESHUTDOWN; 898 899 switch (cmd) { 900 case PI433_IOC_RD_TX_CFG: 901 if (copy_to_user(argp, &instance->tx_cfg, 902 sizeof(struct pi433_tx_cfg))) 903 return -EFAULT; 904 break; 905 case PI433_IOC_WR_TX_CFG: 906 if (copy_from_user(&tx_cfg, argp, sizeof(struct pi433_tx_cfg))) 907 return -EFAULT; 908 mutex_lock(&device->tx_fifo_lock);
909 memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); 910 mutex_unlock(&device->tx_fifo_lock); 911 break; 912 case PI433_IOC_RD_RX_CFG: 913 if (copy_to_user(argp, &device->rx_cfg, 914 sizeof(struct pi433_rx_cfg))) 915 return -EFAULT; 916 break; 917 case PI433_IOC_WR_RX_CFG: 918 mutex_lock(&device->rx_lock); 919 920 /* during pendig read request, change of config not allowed */ 921 if (device->rx_active) { 922 mutex_unlock(&device->rx_lock); 923 return -EAGAIN; 924 } 925 926 if (copy_from_user(&device->rx_cfg, argp, 927 sizeof(struct pi433_rx_cfg))) { 928 mutex_unlock(&device->rx_lock); 929 return -EFAULT; 930 } 931 932 mutex_unlock(&device->rx_lock); 933 break; 934 default: 935 retval = -EINVAL; 936 } 937 938 return retval; 939 } 940
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Hugo, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] [also build test ERROR on v4.17 next-20180613] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hugo-Lefeuvre/staging-pi433-fix-rac... config: x86_64-randconfig-x018-201823 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/staging/pi433/pi433_if.c: In function 'pi433_ioctl':
drivers/staging/pi433/pi433_if.c:909:30: error: 'tx_cfg_buffer' undeclared (first use in this function); did you mean 'sg_copy_buffer'? memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); ^~~~~~~~~~~~~ sg_copy_buffer drivers/staging/pi433/pi433_if.c:909:30: note: each undeclared identifier is reported only once for each function it appears in
vim +909 drivers/staging/pi433/pi433_if.c 876 877 static long 878 pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) 879 { 880 int retval = 0; 881 struct pi433_instance *instance; 882 struct pi433_device *device; 883 struct pi433_tx_cfg tx_cfg; 884 void __user *argp = (void __user *)arg; 885 886 /* Check type and command number */ 887 if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC) 888 return -ENOTTY; 889 890 /* TODO? guard against device removal before, or while, 891 * we issue this ioctl. --> device_get() 892 */ 893 instance = filp->private_data; 894 device = instance->device; 895 896 if (!device) 897 return -ESHUTDOWN; 898 899 switch (cmd) { 900 case PI433_IOC_RD_TX_CFG: 901 if (copy_to_user(argp, &instance->tx_cfg, 902 sizeof(struct pi433_tx_cfg))) 903 return -EFAULT; 904 break; 905 case PI433_IOC_WR_TX_CFG: 906 if (copy_from_user(&tx_cfg, argp, sizeof(struct pi433_tx_cfg))) 907 return -EFAULT; 908 mutex_lock(&device->tx_fifo_lock);
909 memcpy(&instance->tx_cfg, &tx_cfg_buffer, sizeof(struct pi433_tx_cfg)); 910 mutex_unlock(&device->tx_fifo_lock); 911 break; 912 case PI433_IOC_RD_RX_CFG: 913 if (copy_to_user(argp, &device->rx_cfg, 914 sizeof(struct pi433_rx_cfg))) 915 return -EFAULT; 916 break; 917 case PI433_IOC_WR_RX_CFG: 918 mutex_lock(&device->rx_lock); 919 920 /* during pendig read request, change of config not allowed */ 921 if (device->rx_active) { 922 mutex_unlock(&device->rx_lock); 923 return -EAGAIN; 924 } 925 926 if (copy_from_user(&device->rx_cfg, argp, 927 sizeof(struct pi433_rx_cfg))) { 928 mutex_unlock(&device->rx_lock); 929 return -EFAULT; 930 } 931 932 mutex_unlock(&device->rx_lock); 933 break; 934 default: 935 retval = -EINVAL; 936 } 937 938 return retval; 939 } 940
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
participants (2)
-
Hugo Lefeuvre -
kbuild test robot