make 'user_access_begin()' do 'access_ok()'

Originally, the rule used to be that you'd have to do access_ok() separately, and then user_access_begin() before actually doing the direct (optimized) user access. But experience has shown that people then decide not to do access_ok() at all, and instead rely on it being implied by other operations or similar. Which makes it very hard to verify that the access has actually been range-checked. If you use the unsafe direct user accesses, hardware features (either SMAP - Supervisor Mode Access Protection - on x86, or PAN - Privileged Access Never - on ARM) do force you to use user_access_begin(). But nothing really forces the range check. By putting the range check into user_access_begin(), we actually force people to do the right thing (tm), and the range check vill be visible near the actual accesses. We have way too long a history of people trying to avoid them.

Linus Torvalds committed 7 years ago

Commit 594cc251 parent 0b2c8f87 files · +36 −20Browse files

Unified · Split

7 files

arch/x86/include/asm/uaccess.h +8 −1
drivers/gpu/drm/i915/i915_gem_execbuffer.c +13 −2
include/linux/uaccess.h +1 −1
kernel/compat.c +2 −4
kernel/exit.c +2 −4
lib/strncpy_from_user.c +5 −4
lib/strnlen_user.c +5 −4

arch/x86/include/asm/uaccess.h+8−1

  * checking before using them, but you have to surround them with the
  * user_access_begin/end() pair.
  */
-#define user_access_begin()	__uaccess_begin()
+static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
+{
+	if (unlikely(!access_ok(ptr,len)))
+		return 0;
+	__uaccess_begin();
+	return 1;
+}
+#define user_access_begin(a,b)	user_access_begin(a,b)
 #define user_access_end()	__uaccess_end()
 #define unsafe_put_user(x, ptr, err_label)					\

drivers/gpu/drm/i915/i915_gem_execbuffer.c+13−2

 		 * happened we would make the mistake of assuming that the
 		 * relocations were valid.
 		 */
-		user_access_begin();
+		if (!user_access_begin(urelocs, size))
+			goto end_user;
 		for (copied = 0; copied < nreloc; copied++)
 			unsafe_put_user(-1,
 					&urelocs[copied].presumed_offset,
 		unsigned int i;
 		/* Copy the new buffer offsets back to the user's exec list. */
-		user_access_begin();
+		/*
+		 * Note: count * sizeof(*user_exec_list) does not overflow,
+		 * because we checked 'count' in check_buffer_count().
+		 *
+		 * And this range already got effectively checked earlier
+		 * when we did the "copy_from_user()" above.
+		 */
+		if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list)))
+			goto end_user;
 		for (i = 0; i < args->buffer_count; i++) {
 			if (!(exec2_list[i].offset & UPDATE))
 				continue;

include/linux/uaccess.h+1−1

@@ -264,7 +264,7 @@ extern long strncpy_from_unsafe(char dst, const void unsafe_addr, long count); · 263 unmodified lines
264	probe_kernel_read(&retval, addr, sizeof(retval))	264	probe_kernel_read(&retval, addr, sizeof(retval))
265		265
266	#ifndef user_access_begin	266	#ifndef user_access_begin
267	#define user_access_begin() do { } while (0)	267	#define user_access_begin(ptr,len) access_ok(ptr, len)
268	#define user_access_end() do { } while (0)	268	#define user_access_end() do { } while (0)
269	#define unsafe_get_user(x, ptr, err) do { if (unlikely(__get_user(x, ptr))) goto err; } while (0)	269	#define unsafe_get_user(x, ptr, err) do { if (unlikely(__get_user(x, ptr))) goto err; } while (0)
270	#define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0)	270	#define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0)

kernel/compat.c+2−4

 	bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
 	nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
-	if (!access_ok(umask, bitmap_size / 8))
+	if (!user_access_begin(umask, bitmap_size / 8))
 		return -EFAULT;
-	user_access_begin();
 	while (nr_compat_longs > 1) {
 		compat_ulong_t l1, l2;
 		unsafe_get_user(l1, umask++, Efault);
 	bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
 	nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
-	if (!access_ok(umask, bitmap_size / 8))
+	if (!user_access_begin(umask, bitmap_size / 8))
 		return -EFAULT;
-	user_access_begin();
 	while (nr_compat_longs > 1) {
 		unsigned long m = *mask++;
 		unsafe_put_user((compat_ulong_t)m, umask++, Efault);

kernel/exit.c+2−4

 	if (!infop)
 		return err;
-	if (!access_ok(infop, sizeof(*infop)))
+	if (!user_access_begin(infop, sizeof(*infop)))
 		return -EFAULT;
-	user_access_begin();
 	unsafe_put_user(signo, &infop->si_signo, Efault);
 	unsafe_put_user(0, &infop->si_errno, Efault);
 	unsafe_put_user(info.cause, &infop->si_code, Efault);
 	if (!infop)
 		return err;
-	if (!access_ok(infop, sizeof(*infop)))
+	if (!user_access_begin(infop, sizeof(*infop)))
 		return -EFAULT;
-	user_access_begin();
 	unsafe_put_user(signo, &infop->si_signo, Efault);
 	unsafe_put_user(0, &infop->si_errno, Efault);
 	unsafe_put_user(info.cause, &infop->si_code, Efault);

lib/strncpy_from_user.c+5−4

 		kasan_check_write(dst, count);
 		check_object_size(dst, count, false);
-		user_access_begin();
+		if (user_access_begin(src, max)) {
-		retval = do_strncpy_from_user(dst, src, count, max);
+			retval = do_strncpy_from_user(dst, src, count, max);
-		user_access_end();
+			user_access_end();
-		return retval;
+			return retval;
+		}
 	}
 	return -EFAULT;
 }

lib/strnlen_user.c+5−4

 		unsigned long max = max_addr - src_addr;
 		long retval;
-		user_access_begin();
+		if (user_access_begin(str, max)) {
-		retval = do_strnlen_user(str, count, max);
+			retval = do_strnlen_user(str, count, max);
-		user_access_end();
+			user_access_end();
-		return retval;
+			return retval;
+		}
 	}
 	return 0;
 }