Commit 1f5e31d7e55ac7fbd4ec5e5b20c8868b0e4564c9

Authored by Andrea Righi
Committed by Linus Torvalds
1 parent afd8d0f940

fbmem: don't call copy_from/to_user() with mutex held

Avoid calling copy_from/to_user() with fb_info->lock mutex held in fbmem
ioctl().

fb_mmap() is called under mm->mmap_sem (A) held, that also acquires
fb_info->lock (B); fb_ioctl() takes fb_info->lock (B) and does
copy_from/to_user() that might acquire mm->mmap_sem (A), causing a
deadlock.

NOTE: it doesn't push down the fb_info->lock in each own driver's
fb_ioctl(), so there are still potential deadlocks elsewhere.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
Cc: Dave Jones <davej@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Johannes Weiner <hannes@saeurebad.de>
Cc: Krzysztof Helt <krzysztof.h1@wp.pl>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Showing 3 changed files with 98 additions and 72 deletions Side-by-side Diff

drivers/video/fbcmap.c
... ... @@ -250,10 +250,6 @@
250 250 int rc, size = cmap->len * sizeof(u16);
251 251 struct fb_cmap umap;
252 252  
253   - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
254   - !info->fbops->fb_setcmap))
255   - return -EINVAL;
256   -
257 253 memset(&umap, 0, sizeof(struct fb_cmap));
258 254 rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
259 255 if (rc)
260 256  
261 257  
... ... @@ -262,11 +258,23 @@
262 258 copy_from_user(umap.green, cmap->green, size) ||
263 259 copy_from_user(umap.blue, cmap->blue, size) ||
264 260 (cmap->transp && copy_from_user(umap.transp, cmap->transp, size))) {
265   - fb_dealloc_cmap(&umap);
266   - return -EFAULT;
  261 + rc = -EFAULT;
  262 + goto out;
267 263 }
268 264 umap.start = cmap->start;
  265 + if (!lock_fb_info(info)) {
  266 + rc = -ENODEV;
  267 + goto out;
  268 + }
  269 + if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
  270 + !info->fbops->fb_setcmap)) {
  271 + rc = -EINVAL;
  272 + goto out1;
  273 + }
269 274 rc = fb_set_cmap(&umap, info);
  275 +out1:
  276 + unlock_fb_info(info);
  277 +out:
270 278 fb_dealloc_cmap(&umap);
271 279 return rc;
272 280 }
drivers/video/fbmem.c
... ... @@ -1013,132 +1013,139 @@
1013 1013 struct fb_var_screeninfo var;
1014 1014 struct fb_fix_screeninfo fix;
1015 1015 struct fb_con2fbmap con2fb;
  1016 + struct fb_cmap cmap_from;
1016 1017 struct fb_cmap_user cmap;
1017 1018 struct fb_event event;
1018 1019 void __user *argp = (void __user *)arg;
1019 1020 long ret = 0;
1020 1021  
1021   - fb = info->fbops;
1022   - if (!fb)
1023   - return -ENODEV;
1024   -
1025 1022 switch (cmd) {
1026 1023 case FBIOGET_VSCREENINFO:
1027   - ret = copy_to_user(argp, &info->var,
1028   - sizeof(var)) ? -EFAULT : 0;
  1024 + if (!lock_fb_info(info))
  1025 + return -ENODEV;
  1026 + var = info->var;
  1027 + unlock_fb_info(info);
  1028 +
  1029 + ret = copy_to_user(argp, &var, sizeof(var)) ? -EFAULT : 0;
1029 1030 break;
1030 1031 case FBIOPUT_VSCREENINFO:
1031   - if (copy_from_user(&var, argp, sizeof(var))) {
1032   - ret = -EFAULT;
1033   - break;
1034   - }
  1032 + if (copy_from_user(&var, argp, sizeof(var)))
  1033 + return -EFAULT;
  1034 + if (!lock_fb_info(info))
  1035 + return -ENODEV;
1035 1036 acquire_console_sem();
1036 1037 info->flags |= FBINFO_MISC_USEREVENT;
1037 1038 ret = fb_set_var(info, &var);
1038 1039 info->flags &= ~FBINFO_MISC_USEREVENT;
1039 1040 release_console_sem();
1040   - if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
  1041 + unlock_fb_info(info);
  1042 + if (!ret && copy_to_user(argp, &var, sizeof(var)))
1041 1043 ret = -EFAULT;
1042 1044 break;
1043 1045 case FBIOGET_FSCREENINFO:
1044   - ret = copy_to_user(argp, &info->fix,
1045   - sizeof(fix)) ? -EFAULT : 0;
  1046 + if (!lock_fb_info(info))
  1047 + return -ENODEV;
  1048 + fix = info->fix;
  1049 + unlock_fb_info(info);
  1050 +
  1051 + ret = copy_to_user(argp, &fix, sizeof(fix)) ? -EFAULT : 0;
1046 1052 break;
1047 1053 case FBIOPUTCMAP:
1048 1054 if (copy_from_user(&cmap, argp, sizeof(cmap)))
1049   - ret = -EFAULT;
1050   - else
1051   - ret = fb_set_user_cmap(&cmap, info);
  1055 + return -EFAULT;
  1056 + ret = fb_set_user_cmap(&cmap, info);
1052 1057 break;
1053 1058 case FBIOGETCMAP:
1054 1059 if (copy_from_user(&cmap, argp, sizeof(cmap)))
1055   - ret = -EFAULT;
1056   - else
1057   - ret = fb_cmap_to_user(&info->cmap, &cmap);
  1060 + return -EFAULT;
  1061 + if (!lock_fb_info(info))
  1062 + return -ENODEV;
  1063 + cmap_from = info->cmap;
  1064 + unlock_fb_info(info);
  1065 + ret = fb_cmap_to_user(&cmap_from, &cmap);
1058 1066 break;
1059 1067 case FBIOPAN_DISPLAY:
1060   - if (copy_from_user(&var, argp, sizeof(var))) {
1061   - ret = -EFAULT;
1062   - break;
1063   - }
  1068 + if (copy_from_user(&var, argp, sizeof(var)))
  1069 + return -EFAULT;
  1070 + if (!lock_fb_info(info))
  1071 + return -ENODEV;
1064 1072 acquire_console_sem();
1065 1073 ret = fb_pan_display(info, &var);
1066 1074 release_console_sem();
  1075 + unlock_fb_info(info);
1067 1076 if (ret == 0 && copy_to_user(argp, &var, sizeof(var)))
1068   - ret = -EFAULT;
  1077 + return -EFAULT;
1069 1078 break;
1070 1079 case FBIO_CURSOR:
1071 1080 ret = -EINVAL;
1072 1081 break;
1073 1082 case FBIOGET_CON2FBMAP:
1074 1083 if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
1075   - ret = -EFAULT;
1076   - else if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
1077   - ret = -EINVAL;
1078   - else {
1079   - con2fb.framebuffer = -1;
1080   - event.info = info;
1081   - event.data = &con2fb;
1082   - fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP,
1083   - &event);
1084   - ret = copy_to_user(argp, &con2fb,
1085   - sizeof(con2fb)) ? -EFAULT : 0;
1086   - }
  1084 + return -EFAULT;
  1085 + if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
  1086 + return -EINVAL;
  1087 + con2fb.framebuffer = -1;
  1088 + event.data = &con2fb;
  1089 +
  1090 + if (!lock_fb_info(info))
  1091 + return -ENODEV;
  1092 + event.info = info;
  1093 + fb_notifier_call_chain(FB_EVENT_GET_CONSOLE_MAP, &event);
  1094 + unlock_fb_info(info);
  1095 +
  1096 + ret = copy_to_user(argp, &con2fb, sizeof(con2fb)) ? -EFAULT : 0;
1087 1097 break;
1088 1098 case FBIOPUT_CON2FBMAP:
1089   - if (copy_from_user(&con2fb, argp, sizeof(con2fb))) {
1090   - ret = -EFAULT;
1091   - break;
1092   - }
1093   - if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES) {
1094   - ret = -EINVAL;
1095   - break;
1096   - }
1097   - if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX) {
1098   - ret = -EINVAL;
1099   - break;
1100   - }
  1099 + if (copy_from_user(&con2fb, argp, sizeof(con2fb)))
  1100 + return -EFAULT;
  1101 + if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
  1102 + return -EINVAL;
  1103 + if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
  1104 + return -EINVAL;
1101 1105 if (!registered_fb[con2fb.framebuffer])
1102 1106 request_module("fb%d", con2fb.framebuffer);
1103 1107 if (!registered_fb[con2fb.framebuffer]) {
1104 1108 ret = -EINVAL;
1105 1109 break;
1106 1110 }
1107   - event.info = info;
1108 1111 event.data = &con2fb;
  1112 + if (!lock_fb_info(info))
  1113 + return -ENODEV;
  1114 + event.info = info;
1109 1115 ret = fb_notifier_call_chain(FB_EVENT_SET_CONSOLE_MAP,
1110 1116 &event);
  1117 + unlock_fb_info(info);
1111 1118 break;
1112 1119 case FBIOBLANK:
  1120 + if (!lock_fb_info(info))
  1121 + return -ENODEV;
1113 1122 acquire_console_sem();
1114 1123 info->flags |= FBINFO_MISC_USEREVENT;
1115 1124 ret = fb_blank(info, arg);
1116 1125 info->flags &= ~FBINFO_MISC_USEREVENT;
1117 1126 release_console_sem();
1118   - break;;
  1127 + unlock_fb_info(info);
  1128 + break;
1119 1129 default:
1120   - if (fb->fb_ioctl == NULL)
1121   - ret = -ENOTTY;
1122   - else
  1130 + if (!lock_fb_info(info))
  1131 + return -ENODEV;
  1132 + fb = info->fbops;
  1133 + if (fb->fb_ioctl)
1123 1134 ret = fb->fb_ioctl(info, cmd, arg);
  1135 + else
  1136 + ret = -ENOTTY;
  1137 + unlock_fb_info(info);
1124 1138 }
1125 1139 return ret;
1126 1140 }
1127 1141  
1128 1142 static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
1129   -__acquires(&info->lock)
1130   -__releases(&info->lock)
1131 1143 {
1132 1144 struct inode *inode = file->f_path.dentry->d_inode;
1133 1145 int fbidx = iminor(inode);
1134   - struct fb_info *info;
1135   - long ret;
  1146 + struct fb_info *info = registered_fb[fbidx];
1136 1147  
1137   - info = registered_fb[fbidx];
1138   - mutex_lock(&info->lock);
1139   - ret = do_fb_ioctl(info, cmd, arg);
1140   - mutex_unlock(&info->lock);
1141   - return ret;
  1148 + return do_fb_ioctl(info, cmd, arg);
1142 1149 }
1143 1150  
1144 1151 #ifdef CONFIG_COMPAT
... ... @@ -1257,8 +1264,6 @@
1257 1264  
1258 1265 static long fb_compat_ioctl(struct file *file, unsigned int cmd,
1259 1266 unsigned long arg)
1260   -__acquires(&info->lock)
1261   -__releases(&info->lock)
1262 1267 {
1263 1268 struct inode *inode = file->f_path.dentry->d_inode;
1264 1269 int fbidx = iminor(inode);
... ... @@ -1266,7 +1271,6 @@
1266 1271 struct fb_ops *fb = info->fbops;
1267 1272 long ret = -ENOIOCTLCMD;
1268 1273  
1269   - mutex_lock(&info->lock);
1270 1274 switch(cmd) {
1271 1275 case FBIOGET_VSCREENINFO:
1272 1276 case FBIOPUT_VSCREENINFO:
... ... @@ -1292,7 +1296,6 @@
1292 1296 ret = fb->fb_compat_ioctl(info, cmd, arg);
1293 1297 break;
1294 1298 }
1295   - mutex_unlock(&info->lock);
1296 1299 return ret;
1297 1300 }
1298 1301 #endif
... ... @@ -960,6 +960,21 @@
960 960 extern int num_registered_fb;
961 961 extern struct class *fb_class;
962 962  
  963 +static inline int lock_fb_info(struct fb_info *info)
  964 +{
  965 + mutex_lock(&info->lock);
  966 + if (!info->fbops) {
  967 + mutex_unlock(&info->lock);
  968 + return 0;
  969 + }
  970 + return 1;
  971 +}
  972 +
  973 +static inline void unlock_fb_info(struct fb_info *info)
  974 +{
  975 + mutex_unlock(&info->lock);
  976 +}
  977 +
963 978 static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
964 979 u8 *src, u32 s_pitch, u32 height)
965 980 {