[PATCH v3] staging: pi433: fix race condition in pi433_ioctl

[PATCH v3] staging: pi433: fix race condition in pi433_ioctl

[Hugo Lefeuvre]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] staging: pi433: fix race condition in pi433_ioctl

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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-race-condition-in-pi433_ioctl/20180614-091011
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30137 bytes --]

Re: [PATCH v3] staging: pi433: fix race condition in pi433_ioctl

[Hugo Lefeuvre]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

> 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]

Already fixed in v4. Sorry for the noise.

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] staging: pi433: fix race condition in pi433_ioctl

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4521 bytes --]

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-race-condition-in-pi433_ioctl/20180614-091011
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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35175 bytes --]