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 <[email protected]>
---
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-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
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
> 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) | http://www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA