[PATCH 0/3] add fault injection to user memory access functions

[PATCH 0/3] add fault injection to user memory access functions

[albert.linde]

From: Albert van der Linde <alinde@google.com>

The goal of this series is to improve testing of fault-tolerance in
usages of user memory access functions, by adding support for fault
injection.

The first patch adds failure injection capability for usercopy
functions. The second changes usercopy functions to use this new failure
capability (copy_from_user, ...). The third patch adds
get/put/clear_user failures to x86.

Albert van der Linde (3):
  lib, include/linux: add usercopy failure capability
  lib, uaccess: add failure injection to usercopy functions
  x86: add failure injection to get/put/clear_user

 .../fault-injection/fault-injection.rst       | 64 +++++++++++++++++
 arch/x86/include/asm/uaccess.h                | 70 +++++++++++--------
 arch/x86/lib/usercopy_64.c                    |  9 ++-
 include/linux/fault-inject-usercopy.h         | 20 ++++++
 include/linux/uaccess.h                       | 31 ++++++--
 lib/Kconfig.debug                             |  7 ++
 lib/Makefile                                  |  1 +
 lib/fault-inject-usercopy.c                   | 66 +++++++++++++++++
 lib/iov_iter.c                                | 20 +++++-
 lib/strncpy_from_user.c                       |  3 +
 lib/usercopy.c                                | 13 +++-
 11 files changed, 263 insertions(+), 41 deletions(-)
 create mode 100644 include/linux/fault-inject-usercopy.h
 create mode 100644 lib/fault-inject-usercopy.c

-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH 1/3] lib, include/linux: add usercopy failure capability

[albert.linde]

From: Albert van der Linde <alinde@google.com>

Add a failure injection capability to improve testing of fault-tolerance
in usages of user memory access functions.

Adds CONFIG_FAULT_INJECTION_USERCOPY to enable faults in usercopy
functions. By default functions are to either fail with -EFAULT or
return that no bytes were copied. To use partial copies (e.g.,
copy_from_user permits partial failures) the number of bytes not to copy
must be written to /sys/kernel/debug/fail_usercopy/failsize.

Signed-off-by: Albert van der Linde <alinde@google.com>
---
 .../fault-injection/fault-injection.rst       | 64 ++++++++++++++++++
 include/linux/fault-inject-usercopy.h         | 20 ++++++
 lib/Kconfig.debug                             |  7 ++
 lib/Makefile                                  |  1 +
 lib/fault-inject-usercopy.c                   | 66 +++++++++++++++++++
 5 files changed, 158 insertions(+)
 create mode 100644 include/linux/fault-inject-usercopy.h
 create mode 100644 lib/fault-inject-usercopy.c

diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
index f850ad018b70..cf52eabde332 100644
--- a/Documentation/fault-injection/fault-injection.rst
+++ b/Documentation/fault-injection/fault-injection.rst
@@ -16,6 +16,10 @@ Available fault injection capabilities
 
   injects page allocation failures. (alloc_pages(), get_free_pages(), ...)
 
+- fail_usercopy
+
+  injects failures in user memory access functions. (copy_from_user(), get_user(), ...)
+
 - fail_futex
 
   injects futex deadlock and uaddr fault errors.
@@ -138,6 +142,12 @@ configuration of fault-injection capabilities.
 	specifies the minimum page allocation order to be injected
 	failures.
 
+- /sys/kernel/debug/fail_usercopy/failsize:
+
+	specifies the error code to return or amount of bytes not to copy.
+	If set to 0 then -EFAULT is returned instead. Positive values can be
+	used to inject partial copies (copy_from_user(), ...)
+
 - /sys/kernel/debug/fail_futex/ignore-private:
 
 	Format: { 'Y' | 'N' }
@@ -177,6 +187,7 @@ use the boot option::
 
 	failslab=
 	fail_page_alloc=
+	fail_usercopy=
 	fail_make_request=
 	fail_futex=
 	mmc_core.fail_request=<interval>,<probability>,<space>,<times>
@@ -383,6 +394,59 @@ allocation failure::
 		./tools/testing/fault-injection/failcmd.sh --times=100 \
 		-- make -C tools/testing/selftests/ run_tests
 
+
+Fail usercopy functions
+---------------------------------
+
+The following code fails the usercopy call in stat: copy_to_user is only
+partially completed.
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/sendfile.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <time.h>
+#include <unistd.h>
+
+void inject_size()
+{
+	int fd = open("/sys/kernel/debug/fail_usercopy/failsize", O_RDWR);
+	char buf[16];
+	sprintf(buf, "%d", 60);
+	write(fd, buf, strlen(buf));
+}
+
+void inject_fault()
+{
+	int fd = open("/proc/thread-self/fail-nth", O_RDWR);
+	char buf[16];
+	sprintf(buf, "%d", 4);
+	write(fd, buf, strlen(buf));
+}
+
+int main()
+{
+	struct stat sb;
+	inject_size();
+	inject_fault();
+	stat(".", &sb);
+	printf("Stat error code:          %d\n", errno);
+	printf("Last status change:       %s", ctime(&sb.st_ctime));
+	printf("Last file access:         %s", ctime(&sb.st_atime));
+	printf("Last file modification:   %s", ctime(&sb.st_mtime));
+	return 0;
+}
+
+Example output:
+Stat error code:          14
+Last status change:       Thu Jan  1 00:00:00 1970
+Last file access:         Tue Aug 18 14:09:34 2020
+Last file modification:   Sun Dec 12 00:19:57 2989041
+
 Systematic faults using fail-nth
 ---------------------------------
 
diff --git a/include/linux/fault-inject-usercopy.h b/include/linux/fault-inject-usercopy.h
new file mode 100644
index 000000000000..95dad8ddb56f
--- /dev/null
+++ b/include/linux/fault-inject-usercopy.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This header provides a wrapper for partial failures on usercopy functions.
+ * For usage see Documentation/fault-injection/fault-injection.rst
+ */
+#ifndef __LINUX_FAULT_INJECT_USERCOPY_H__
+#define __LINUX_FAULT_INJECT_USERCOPY_H__
+
+#ifdef CONFIG_FAULT_INJECTION_USERCOPY
+
+long should_fail_usercopy(unsigned long n);
+
+#else
+
+static inline long should_fail_usercopy(unsigned long n) { return 0; }
+
+#endif /* CONFIG_FAULT_INJECTION_USERCOPY */
+
+#endif /* __LINUX_FAULT_INJECT_USERCOPY_H__ */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e068c3c7189a..bd0ec3ddb459 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1770,6 +1770,13 @@ config FAIL_PAGE_ALLOC
 	help
 	  Provide fault-injection capability for alloc_pages().
 
+config FAULT_INJECTION_USERCOPY
+	bool "Fault injection capability for usercopy functions"
+	depends on FAULT_INJECTION
+	help
+	  Provides fault-injection capability to inject failures and
+	  partial copies in usercopy functions.
+
 config FAIL_MAKE_REQUEST
 	bool "Fault-injection capability for disk IO"
 	depends on FAULT_INJECTION && BLOCK
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..18daad2bc606 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -207,6 +207,7 @@ obj-$(CONFIG_AUDIT_COMPAT_GENERIC) += compat_audit.o
 
 obj-$(CONFIG_IOMMU_HELPER) += iommu-helper.o
 obj-$(CONFIG_FAULT_INJECTION) += fault-inject.o
+obj-$(CONFIG_FAULT_INJECTION_USERCOPY) += fault-inject-usercopy.o
 obj-$(CONFIG_NOTIFIER_ERROR_INJECTION) += notifier-error-inject.o
 obj-$(CONFIG_PM_NOTIFIER_ERROR_INJECT) += pm-notifier-error-inject.o
 obj-$(CONFIG_NETDEV_NOTIFIER_ERROR_INJECT) += netdev-notifier-error-inject.o
diff --git a/lib/fault-inject-usercopy.c b/lib/fault-inject-usercopy.c
new file mode 100644
index 000000000000..c151e0817ca7
--- /dev/null
+++ b/lib/fault-inject-usercopy.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/fault-inject.h>
+#include <linux/fault-inject-usercopy.h>
+#include <linux/random.h>
+
+static struct {
+	struct fault_attr attr;
+	u32 failsize;
+} fail_usercopy = {
+	.attr = FAULT_ATTR_INITIALIZER,
+	.failsize = 0,
+};
+
+static int __init setup_fail_usercopy(char *str)
+{
+	return setup_fault_attr(&fail_usercopy.attr, str);
+}
+__setup("fail_usercopy=", setup_fail_usercopy);
+
+#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
+
+static int __init fail_usercopy_debugfs(void)
+{
+	umode_t mode = S_IFREG | 0600;
+	struct dentry *dir;
+
+	dir = fault_create_debugfs_attr("fail_usercopy", NULL,
+					&fail_usercopy.attr);
+	if (IS_ERR(dir))
+		return PTR_ERR(dir);
+
+	debugfs_create_u32("failsize", mode, dir,
+			   &fail_usercopy.failsize);
+	return 0;
+}
+
+late_initcall(fail_usercopy_debugfs);
+
+#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
+
+/**
+ * should_fail_usercopy() - Failure code or amount of bytes not to copy.
+ * @n: Size of the original copy call.
+ *
+ * The general idea is to have a method which returns the amount of bytes not
+ * to copy, a failure to return, or 0 if the calling function should progress
+ * without a failure. E.g., copy_{to,from}_user should NOT copy the amount of
+ * bytes returned by should_fail_usercopy, returning this value (in addition
+ * to any bytes that could actually not be copied) or a failure.
+ *
+ * Return: one of:
+ * negative, failure to return;
+ * 0, progress normally;
+ * a number in ]0, n], the number of bytes not to copy.
+ *
+ */
+long should_fail_usercopy(unsigned long n)
+{
+	if (should_fail(&fail_usercopy.attr, n)) {
+		if (fail_usercopy.failsize > 0)
+			return fail_usercopy.failsize % (n + 1);
+		return -EFAULT;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(should_fail_usercopy);
-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH 2/3] lib, uaccess: add failure injection to usercopy functions

[albert.linde]

From: Albert van der Linde <alinde@google.com>

To test fault-tolerance of usercopy accesses, introduce fault injection
in usercopy functions.

Adds failure injection to usercopy functions. If a failure is expected
we return either the failure or the total amount of bytes. As many
usercopy functions can fail partially, permit also partial failures:
e.g., copy_from_user can fail copying between 0 and n.

Signed-off-by: Albert van der Linde <alinde@google.com>
---
 include/linux/uaccess.h | 31 ++++++++++++++++++++++++++-----
 lib/iov_iter.c          | 20 +++++++++++++++++---
 lib/strncpy_from_user.c |  3 +++
 lib/usercopy.c          | 13 +++++++++++--
 4 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 94b285411659..79e736077ef4 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_UACCESS_H__
 #define __LINUX_UACCESS_H__
 
+#include <linux/fault-inject-usercopy.h>
 #include <linux/instrumented.h>
 #include <linux/sched.h>
 #include <linux/thread_info.h>
@@ -82,10 +83,14 @@ __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
 static __always_inline __must_check unsigned long
 __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	instrument_copy_from_user(to, from, n);
 	check_object_size(to, n, false);
-	return raw_copy_from_user(to, from, n);
+	return not_copied + raw_copy_from_user(to, from, n - not_copied);
 }
 
 /**
@@ -104,18 +109,26 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
 static __always_inline __must_check unsigned long
 __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	instrument_copy_to_user(to, from, n);
 	check_object_size(from, n, true);
-	return raw_copy_to_user(to, from, n);
+	return not_copied + raw_copy_to_user(to, from, n - not_copied);
 }
 
 static __always_inline __must_check unsigned long
 __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	instrument_copy_to_user(to, from, n);
 	check_object_size(from, n, true);
-	return raw_copy_to_user(to, from, n);
+	return not_copied + raw_copy_to_user(to, from, n - not_copied);
 }
 
 #ifdef INLINE_COPY_FROM_USER
@@ -123,10 +136,14 @@ static inline __must_check unsigned long
 _copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	unsigned long res = n;
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	if (likely(access_ok(from, n))) {
 		instrument_copy_from_user(to, from, n);
-		res = raw_copy_from_user(to, from, n);
+		res = not_copied + raw_copy_from_user(to, from, n - not_copied);
 	}
 	if (unlikely(res))
 		memset(to + (n - res), 0, res);
@@ -141,10 +158,14 @@ _copy_from_user(void *, const void __user *, unsigned long);
 static inline __must_check unsigned long
 _copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	if (access_ok(to, n)) {
 		instrument_copy_to_user(to, from, n);
-		n = raw_copy_to_user(to, from, n);
+		n = not_copied + raw_copy_to_user(to, from, n - not_copied);
 	}
 	return n;
 }
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 5e40786c8f12..c55c9bff9b0c 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -2,6 +2,7 @@
 #include <crypto/hash.h>
 #include <linux/export.h>
 #include <linux/bvec.h>
+#include <linux/fault-inject-usercopy.h>
 #include <linux/uio.h>
 #include <linux/pagemap.h>
 #include <linux/slab.h>
@@ -139,18 +140,26 @@
 
 static int copyout(void __user *to, const void *from, size_t n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		return not_copied;
 	if (access_ok(to, n)) {
 		instrument_copy_to_user(to, from, n);
-		n = raw_copy_to_user(to, from, n);
+		n = not_copied + raw_copy_to_user(to, from, n - not_copied);
 	}
 	return n;
 }
 
 static int copyin(void *to, const void __user *from, size_t n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		return not_copied;
 	if (access_ok(from, n)) {
 		instrument_copy_from_user(to, from, n);
-		n = raw_copy_from_user(to, from, n);
+		n = not_copied + raw_copy_from_user(to, from, n - not_copied);
 	}
 	return n;
 }
@@ -640,9 +649,14 @@ EXPORT_SYMBOL(_copy_to_iter);
 #ifdef CONFIG_ARCH_HAS_UACCESS_MCSAFE
 static int copyout_mcsafe(void __user *to, const void *from, size_t n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		return not_copied;
 	if (access_ok(to, n)) {
 		instrument_copy_to_user(to, from, n);
-		n = copy_to_user_mcsafe((__force void *) to, from, n);
+		n = not_copied + copy_to_user_mcsafe((__force void *) to,
+			       from, n - not_copied);
 	}
 	return n;
 }
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 34696a348864..f65973007931 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/compiler.h>
 #include <linux/export.h>
+#include <linux/fault-inject-usercopy.h>
 #include <linux/kasan-checks.h>
 #include <linux/thread_info.h>
 #include <linux/uaccess.h>
@@ -101,6 +102,8 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	might_fault();
 	if (unlikely(count <= 0))
 		return 0;
+	if (should_fail_usercopy(count))
+		return -EFAULT;
 
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)untagged_addr(src);
diff --git a/lib/usercopy.c b/lib/usercopy.c
index b26509f112f9..e79716b4634b 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/bitops.h>
+#include <linux/fault-inject-usercopy.h>
 #include <linux/instrumented.h>
 #include <linux/uaccess.h>
 
@@ -9,10 +10,14 @@
 unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	unsigned long res = n;
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	if (likely(access_ok(from, n))) {
 		instrument_copy_from_user(to, from, n);
-		res = raw_copy_from_user(to, from, n);
+		res = not_copied + raw_copy_from_user(to, from, n - not_copied);
 	}
 	if (unlikely(res))
 		memset(to + (n - res), 0, res);
@@ -24,10 +29,14 @@ EXPORT_SYMBOL(_copy_from_user);
 #ifndef INLINE_COPY_TO_USER
 unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
 	might_fault();
 	if (likely(access_ok(to, n))) {
 		instrument_copy_to_user(to, from, n);
-		n = raw_copy_to_user(to, from, n);
+		n = not_copied + raw_copy_to_user(to, from, n - not_copied);
 	}
 	return n;
 }
-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH 3/3] x86: add failure injection to get/put/clear_user

[albert.linde]

From: Albert van der Linde <alinde@google.com>

To test fault-tolerance of usercopy accesses in x86, add support for
fault injection.

Make both put_user() and get_user() fail with -EFAULT, and clear_user()
fail by partially clearing fewer bytes.

Signed-off-by: Albert van der Linde <alinde@google.com>
---
 arch/x86/include/asm/uaccess.h | 70 +++++++++++++++++++---------------
 arch/x86/lib/usercopy_64.c     |  9 ++++-
 2 files changed, 48 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ecefaffd15d4..dacb6105831e 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -5,6 +5,7 @@
  * User space memory access functions
  */
 #include <linux/compiler.h>
+#include <linux/fault-inject-usercopy.h>
 #include <linux/kasan-checks.h>
 #include <linux/string.h>
 #include <asm/asm.h>
@@ -174,12 +175,17 @@ extern int __get_user_bad(void);
 	int __ret_gu;							\
 	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
 	__chk_user_ptr(ptr);						\
-	might_fault();							\
-	asm volatile("call __get_user_%P4"				\
-		     : "=a" (__ret_gu), "=r" (__val_gu),		\
+	if (should_fail_usercopy(sizeof(*(ptr)))) {			\
+		(x) = 0;						\
+		__ret_gu = -EFAULT;					\
+	} else {							\
+		might_fault();						\
+		asm volatile("call __get_user_%P4"			\
+			: "=a" (__ret_gu), "=r" (__val_gu),		\
 			ASM_CALL_CONSTRAINT				\
-		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
-	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
+			: "0" (ptr), "i" (sizeof(*(ptr))));		\
+		(x) = (__force __typeof__(*(ptr))) __val_gu;		\
+	}								\
 	__builtin_expect(__ret_gu, 0);					\
 })
 
@@ -236,31 +242,35 @@ extern void __put_user_8(void);
  *
  * Return: zero on success, or -EFAULT on error.
  */
-#define put_user(x, ptr)					\
-({								\
-	int __ret_pu;						\
-	__typeof__(*(ptr)) __pu_val;				\
-	__chk_user_ptr(ptr);					\
-	might_fault();						\
-	__pu_val = x;						\
-	switch (sizeof(*(ptr))) {				\
-	case 1:							\
-		__put_user_x(1, __pu_val, ptr, __ret_pu);	\
-		break;						\
-	case 2:							\
-		__put_user_x(2, __pu_val, ptr, __ret_pu);	\
-		break;						\
-	case 4:							\
-		__put_user_x(4, __pu_val, ptr, __ret_pu);	\
-		break;						\
-	case 8:							\
-		__put_user_x8(__pu_val, ptr, __ret_pu);		\
-		break;						\
-	default:						\
-		__put_user_x(X, __pu_val, ptr, __ret_pu);	\
-		break;						\
-	}							\
-	__builtin_expect(__ret_pu, 0);				\
+#define put_user(x, ptr)						\
+({									\
+	int __ret_pu;							\
+	__typeof__(*(ptr)) __pu_val;					\
+	__chk_user_ptr(ptr);						\
+	might_fault();							\
+	__pu_val = x;							\
+	if (should_fail_usercopy(sizeof(*(ptr)))) {			\
+		__ret_pu = -EFAULT;					\
+	} else {							\
+		switch (sizeof(*(ptr))) {				\
+		case 1:							\
+			__put_user_x(1, __pu_val, ptr, __ret_pu);	\
+			break;						\
+		case 2:							\
+			__put_user_x(2, __pu_val, ptr, __ret_pu);	\
+			break;						\
+		case 4:							\
+			__put_user_x(4, __pu_val, ptr, __ret_pu);	\
+			break;						\
+		case 8:							\
+			__put_user_x8(__pu_val, ptr, __ret_pu);		\
+			break;						\
+		default:						\
+			__put_user_x(X, __pu_val, ptr, __ret_pu);	\
+			break;						\
+		}							\
+	}								\
+	__builtin_expect(__ret_pu, 0);					\
 })
 
 #define __put_user_size(x, ptr, size, label)				\
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index b0dfac3d3df7..b749590d753e 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -7,6 +7,7 @@
  * Copyright 2002 Andi Kleen <ak@suse.de>
  */
 #include <linux/export.h>
+#include <linux/fault-inject-usercopy.h>
 #include <linux/uaccess.h>
 #include <linux/highmem.h>
 
@@ -50,8 +51,14 @@ EXPORT_SYMBOL(__clear_user);
 
 unsigned long clear_user(void __user *to, unsigned long n)
 {
+	long not_copied = should_fail_usercopy(n);
+
+	if (not_copied < 0)
+		not_copied = n;
+
 	if (access_ok(to, n))
-		return __clear_user(to, n);
+		return not_copied + __clear_user(to, n - not_copied);
+
 	return n;
 }
 EXPORT_SYMBOL(clear_user);
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH 2/3] lib, uaccess: add failure injection to usercopy functions

[Dmitry Vyukov]

On Fri, Aug 21, 2020 at 12:50 PM <albert.linde@gmail.com> wrote:
>
> From: Albert van der Linde <alinde@google.com>
>
> To test fault-tolerance of usercopy accesses, introduce fault injection
> in usercopy functions.
>
> Adds failure injection to usercopy functions. If a failure is expected
> we return either the failure or the total amount of bytes. As many
> usercopy functions can fail partially, permit also partial failures:
> e.g., copy_from_user can fail copying between 0 and n.
>
> Signed-off-by: Albert van der Linde <alinde@google.com>
> ---
>  include/linux/uaccess.h | 31 ++++++++++++++++++++++++++-----
>  lib/iov_iter.c          | 20 +++++++++++++++++---
>  lib/strncpy_from_user.c |  3 +++
>  lib/usercopy.c          | 13 +++++++++++--
>  4 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 94b285411659..79e736077ef4 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -2,6 +2,7 @@
>  #ifndef __LINUX_UACCESS_H__
>  #define __LINUX_UACCESS_H__
>
> +#include <linux/fault-inject-usercopy.h>
>  #include <linux/instrumented.h>
>  #include <linux/sched.h>
>  #include <linux/thread_info.h>
> @@ -82,10 +83,14 @@ __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
>  static __always_inline __must_check unsigned long
>  __copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;

It feels that this "if (not_copied < 0)" part should be factored into
some common place because it's duplicated lots of times.
Maybe we need a separate helper similar to should_fail_usercopy that
does not return negative values; or make should_fail_usercopy accept a
flag to return -EFAULT or n; or maybe we simply make
should_fail_usercopy return n instead of -EFAULT?

Also, why do we continue when we already know that we should fail it?
In some places we don't, but in most we still run the actual
raw_copy_from_user. What's the rationale? Can we return early in all
cases?

>         might_fault();
>         instrument_copy_from_user(to, from, n);
>         check_object_size(to, n, false);
> -       return raw_copy_from_user(to, from, n);
> +       return not_copied + raw_copy_from_user(to, from, n - not_copied);

Is returning "not_copied + raw_copy_from_user" fine in all cases?
We can return more than the number of bytes we were asked to copy,
which may be surprising for some callers. Or we can both return an
error and do the copy, which may be surprising for userspace.

>  }
>
>  /**
> @@ -104,18 +109,26 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
>  static __always_inline __must_check unsigned long
>  __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         instrument_copy_to_user(to, from, n);
>         check_object_size(from, n, true);
> -       return raw_copy_to_user(to, from, n);
> +       return not_copied + raw_copy_to_user(to, from, n - not_copied);
>  }
>
>  static __always_inline __must_check unsigned long
>  __copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         might_fault();
>         instrument_copy_to_user(to, from, n);
>         check_object_size(from, n, true);
> -       return raw_copy_to_user(to, from, n);
> +       return not_copied + raw_copy_to_user(to, from, n - not_copied);
>  }
>
>  #ifdef INLINE_COPY_FROM_USER
> @@ -123,10 +136,14 @@ static inline __must_check unsigned long
>  _copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
>         unsigned long res = n;
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         might_fault();
>         if (likely(access_ok(from, n))) {
>                 instrument_copy_from_user(to, from, n);
> -               res = raw_copy_from_user(to, from, n);
> +               res = not_copied + raw_copy_from_user(to, from, n - not_copied);
>         }
>         if (unlikely(res))
>                 memset(to + (n - res), 0, res);
> @@ -141,10 +158,14 @@ _copy_from_user(void *, const void __user *, unsigned long);
>  static inline __must_check unsigned long
>  _copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         might_fault();
>         if (access_ok(to, n)) {
>                 instrument_copy_to_user(to, from, n);
> -               n = raw_copy_to_user(to, from, n);
> +               n = not_copied + raw_copy_to_user(to, from, n - not_copied);
>         }
>         return n;
>  }
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 5e40786c8f12..c55c9bff9b0c 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -2,6 +2,7 @@
>  #include <crypto/hash.h>
>  #include <linux/export.h>
>  #include <linux/bvec.h>
> +#include <linux/fault-inject-usercopy.h>
>  #include <linux/uio.h>
>  #include <linux/pagemap.h>
>  #include <linux/slab.h>
> @@ -139,18 +140,26 @@
>
>  static int copyout(void __user *to, const void *from, size_t n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               return not_copied;
>         if (access_ok(to, n)) {
>                 instrument_copy_to_user(to, from, n);
> -               n = raw_copy_to_user(to, from, n);
> +               n = not_copied + raw_copy_to_user(to, from, n - not_copied);
>         }
>         return n;
>  }
>
>  static int copyin(void *to, const void __user *from, size_t n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               return not_copied;
>         if (access_ok(from, n)) {
>                 instrument_copy_from_user(to, from, n);
> -               n = raw_copy_from_user(to, from, n);
> +               n = not_copied + raw_copy_from_user(to, from, n - not_copied);
>         }
>         return n;
>  }
> @@ -640,9 +649,14 @@ EXPORT_SYMBOL(_copy_to_iter);
>  #ifdef CONFIG_ARCH_HAS_UACCESS_MCSAFE
>  static int copyout_mcsafe(void __user *to, const void *from, size_t n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               return not_copied;
>         if (access_ok(to, n)) {
>                 instrument_copy_to_user(to, from, n);
> -               n = copy_to_user_mcsafe((__force void *) to, from, n);
> +               n = not_copied + copy_to_user_mcsafe((__force void *) to,
> +                              from, n - not_copied);
>         }
>         return n;
>  }
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 34696a348864..f65973007931 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/compiler.h>
>  #include <linux/export.h>
> +#include <linux/fault-inject-usercopy.h>
>  #include <linux/kasan-checks.h>
>  #include <linux/thread_info.h>
>  #include <linux/uaccess.h>
> @@ -101,6 +102,8 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
>         might_fault();
>         if (unlikely(count <= 0))
>                 return 0;
> +       if (should_fail_usercopy(count))
> +               return -EFAULT;
>
>         max_addr = user_addr_max();
>         src_addr = (unsigned long)untagged_addr(src);
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index b26509f112f9..e79716b4634b 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/bitops.h>
> +#include <linux/fault-inject-usercopy.h>
>  #include <linux/instrumented.h>
>  #include <linux/uaccess.h>
>
> @@ -9,10 +10,14 @@
>  unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
>         unsigned long res = n;
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         might_fault();
>         if (likely(access_ok(from, n))) {
>                 instrument_copy_from_user(to, from, n);
> -               res = raw_copy_from_user(to, from, n);
> +               res = not_copied + raw_copy_from_user(to, from, n - not_copied);
>         }
>         if (unlikely(res))
>                 memset(to + (n - res), 0, res);
> @@ -24,10 +29,14 @@ EXPORT_SYMBOL(_copy_from_user);
>  #ifndef INLINE_COPY_TO_USER
>  unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n)
>  {
> +       long not_copied = should_fail_usercopy(n);
> +
> +       if (not_copied < 0)
> +               not_copied = n;
>         might_fault();
>         if (likely(access_ok(to, n))) {
>                 instrument_copy_to_user(to, from, n);
> -               n = raw_copy_to_user(to, from, n);
> +               n = not_copied + raw_copy_to_user(to, from, n - not_copied);
>         }
>         return n;
>  }
> --
> 2.28.0.297.g1956fa8f8d-goog
>

Re: [PATCH 1/3] lib, include/linux: add usercopy failure capability

[Dmitry Vyukov]

On Fri, Aug 21, 2020 at 12:50 PM <albert.linde@gmail.com> wrote:
>
> From: Albert van der Linde <alinde@google.com>
>
> Add a failure injection capability to improve testing of fault-tolerance
> in usages of user memory access functions.
>
> Adds CONFIG_FAULT_INJECTION_USERCOPY to enable faults in usercopy
> functions. By default functions are to either fail with -EFAULT or
> return that no bytes were copied. To use partial copies (e.g.,
> copy_from_user permits partial failures) the number of bytes not to copy
> must be written to /sys/kernel/debug/fail_usercopy/failsize.
>
> Signed-off-by: Albert van der Linde <alinde@google.com>
> ---
>  .../fault-injection/fault-injection.rst       | 64 ++++++++++++++++++
>  include/linux/fault-inject-usercopy.h         | 20 ++++++
>  lib/Kconfig.debug                             |  7 ++
>  lib/Makefile                                  |  1 +
>  lib/fault-inject-usercopy.c                   | 66 +++++++++++++++++++
>  5 files changed, 158 insertions(+)
>  create mode 100644 include/linux/fault-inject-usercopy.h
>  create mode 100644 lib/fault-inject-usercopy.c
>
> diff --git a/Documentation/fault-injection/fault-injection.rst b/Documentation/fault-injection/fault-injection.rst
> index f850ad018b70..cf52eabde332 100644
> --- a/Documentation/fault-injection/fault-injection.rst
> +++ b/Documentation/fault-injection/fault-injection.rst
> @@ -16,6 +16,10 @@ Available fault injection capabilities
>
>    injects page allocation failures. (alloc_pages(), get_free_pages(), ...)
>
> +- fail_usercopy
> +
> +  injects failures in user memory access functions. (copy_from_user(), get_user(), ...)
> +
>  - fail_futex
>
>    injects futex deadlock and uaddr fault errors.
> @@ -138,6 +142,12 @@ configuration of fault-injection capabilities.
>         specifies the minimum page allocation order to be injected
>         failures.
>
> +- /sys/kernel/debug/fail_usercopy/failsize:
> +
> +       specifies the error code to return or amount of bytes not to copy.
> +       If set to 0 then -EFAULT is returned instead. Positive values can be
> +       used to inject partial copies (copy_from_user(), ...)
> +
>  - /sys/kernel/debug/fail_futex/ignore-private:
>
>         Format: { 'Y' | 'N' }
> @@ -177,6 +187,7 @@ use the boot option::
>
>         failslab=
>         fail_page_alloc=
> +       fail_usercopy=
>         fail_make_request=
>         fail_futex=
>         mmc_core.fail_request=<interval>,<probability>,<space>,<times>
> @@ -383,6 +394,59 @@ allocation failure::
>                 ./tools/testing/fault-injection/failcmd.sh --times=100 \
>                 -- make -C tools/testing/selftests/ run_tests
>
> +
> +Fail usercopy functions
> +---------------------------------
> +
> +The following code fails the usercopy call in stat: copy_to_user is only
> +partially completed.
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/sendfile.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +void inject_size()
> +{
> +       int fd = open("/sys/kernel/debug/fail_usercopy/failsize", O_RDWR);
> +       char buf[16];
> +       sprintf(buf, "%d", 60);
> +       write(fd, buf, strlen(buf));
> +}
> +
> +void inject_fault()
> +{
> +       int fd = open("/proc/thread-self/fail-nth", O_RDWR);
> +       char buf[16];
> +       sprintf(buf, "%d", 4);
> +       write(fd, buf, strlen(buf));
> +}
> +
> +int main()
> +{
> +       struct stat sb;
> +       inject_size();
> +       inject_fault();
> +       stat(".", &sb);
> +       printf("Stat error code:          %d\n", errno);
> +       printf("Last status change:       %s", ctime(&sb.st_ctime));
> +       printf("Last file access:         %s", ctime(&sb.st_atime));
> +       printf("Last file modification:   %s", ctime(&sb.st_mtime));
> +       return 0;
> +}
> +
> +Example output:
> +Stat error code:          14
> +Last status change:       Thu Jan  1 00:00:00 1970
> +Last file access:         Tue Aug 18 14:09:34 2020
> +Last file modification:   Sun Dec 12 00:19:57 2989041
> +
>  Systematic faults using fail-nth
>  ---------------------------------
>
> diff --git a/include/linux/fault-inject-usercopy.h b/include/linux/fault-inject-usercopy.h
> new file mode 100644
> index 000000000000..95dad8ddb56f
> --- /dev/null
> +++ b/include/linux/fault-inject-usercopy.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * This header provides a wrapper for partial failures on usercopy functions.
> + * For usage see Documentation/fault-injection/fault-injection.rst
> + */
> +#ifndef __LINUX_FAULT_INJECT_USERCOPY_H__
> +#define __LINUX_FAULT_INJECT_USERCOPY_H__
> +
> +#ifdef CONFIG_FAULT_INJECTION_USERCOPY
> +
> +long should_fail_usercopy(unsigned long n);
> +
> +#else
> +
> +static inline long should_fail_usercopy(unsigned long n) { return 0; }
> +
> +#endif /* CONFIG_FAULT_INJECTION_USERCOPY */
> +
> +#endif /* __LINUX_FAULT_INJECT_USERCOPY_H__ */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e068c3c7189a..bd0ec3ddb459 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1770,6 +1770,13 @@ config FAIL_PAGE_ALLOC
>         help
>           Provide fault-injection capability for alloc_pages().
>
> +config FAULT_INJECTION_USERCOPY
> +       bool "Fault injection capability for usercopy functions"
> +       depends on FAULT_INJECTION
> +       help
> +         Provides fault-injection capability to inject failures and
> +         partial copies in usercopy functions.
> +
>  config FAIL_MAKE_REQUEST
>         bool "Fault-injection capability for disk IO"
>         depends on FAULT_INJECTION && BLOCK
> diff --git a/lib/Makefile b/lib/Makefile
> index a4a4c6864f51..18daad2bc606 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -207,6 +207,7 @@ obj-$(CONFIG_AUDIT_COMPAT_GENERIC) += compat_audit.o
>
>  obj-$(CONFIG_IOMMU_HELPER) += iommu-helper.o
>  obj-$(CONFIG_FAULT_INJECTION) += fault-inject.o
> +obj-$(CONFIG_FAULT_INJECTION_USERCOPY) += fault-inject-usercopy.o
>  obj-$(CONFIG_NOTIFIER_ERROR_INJECTION) += notifier-error-inject.o
>  obj-$(CONFIG_PM_NOTIFIER_ERROR_INJECT) += pm-notifier-error-inject.o
>  obj-$(CONFIG_NETDEV_NOTIFIER_ERROR_INJECT) += netdev-notifier-error-inject.o
> diff --git a/lib/fault-inject-usercopy.c b/lib/fault-inject-usercopy.c
> new file mode 100644
> index 000000000000..c151e0817ca7
> --- /dev/null
> +++ b/lib/fault-inject-usercopy.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/fault-inject.h>
> +#include <linux/fault-inject-usercopy.h>
> +#include <linux/random.h>
> +
> +static struct {
> +       struct fault_attr attr;
> +       u32 failsize;
> +} fail_usercopy = {
> +       .attr = FAULT_ATTR_INITIALIZER,
> +       .failsize = 0,
> +};
> +
> +static int __init setup_fail_usercopy(char *str)
> +{
> +       return setup_fault_attr(&fail_usercopy.attr, str);
> +}
> +__setup("fail_usercopy=", setup_fail_usercopy);
> +
> +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> +
> +static int __init fail_usercopy_debugfs(void)
> +{
> +       umode_t mode = S_IFREG | 0600;
> +       struct dentry *dir;
> +
> +       dir = fault_create_debugfs_attr("fail_usercopy", NULL,
> +                                       &fail_usercopy.attr);
> +       if (IS_ERR(dir))
> +               return PTR_ERR(dir);
> +
> +       debugfs_create_u32("failsize", mode, dir,
> +                          &fail_usercopy.failsize);

Marco, what's the right way to annotate these concurrent accesses for KCSAN?

> +       return 0;
> +}
> +
> +late_initcall(fail_usercopy_debugfs);
> +
> +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
> +
> +/**
> + * should_fail_usercopy() - Failure code or amount of bytes not to copy.
> + * @n: Size of the original copy call.
> + *
> + * The general idea is to have a method which returns the amount of bytes not
> + * to copy, a failure to return, or 0 if the calling function should progress
> + * without a failure. E.g., copy_{to,from}_user should NOT copy the amount of
> + * bytes returned by should_fail_usercopy, returning this value (in addition
> + * to any bytes that could actually not be copied) or a failure.
> + *
> + * Return: one of:
> + * negative, failure to return;
> + * 0, progress normally;
> + * a number in ]0, n], the number of bytes not to copy.
> + *
> + */
> +long should_fail_usercopy(unsigned long n)
> +{
> +       if (should_fail(&fail_usercopy.attr, n)) {
> +               if (fail_usercopy.failsize > 0)
> +                       return fail_usercopy.failsize % (n + 1);

What's the use-case for this fail_usercopy.failsize? How is it
supposed to be used?
It seems that setting an exact value only makes sense if you know the
exact failure site (and size it will use). But it's global and not
per-task.
It does not allow systematic enumeration of all return values because
there is no way to understand that we tried all possible return values
for a particular usercopy. And it also can "undo" failure if %(n+1)
yields 0, right? Maybe it should saturate?


> +               return -EFAULT;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(should_fail_usercopy);
> --
> 2.28.0.297.g1956fa8f8d-goog
>

Re: [PATCH 1/3] lib, include/linux: add usercopy failure capability

[Marco Elver]

On Fri, Aug 21, 2020 at 01:51PM +0200, Dmitry Vyukov wrote:
...
> > +++ b/lib/fault-inject-usercopy.c
> > @@ -0,0 +1,66 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/fault-inject.h>
> > +#include <linux/fault-inject-usercopy.h>
> > +#include <linux/random.h>
> > +
> > +static struct {
> > +       struct fault_attr attr;
> > +       u32 failsize;
> > +} fail_usercopy = {
> > +       .attr = FAULT_ATTR_INITIALIZER,
> > +       .failsize = 0,
> > +};
> > +
> > +static int __init setup_fail_usercopy(char *str)
> > +{
> > +       return setup_fault_attr(&fail_usercopy.attr, str);
> > +}
> > +__setup("fail_usercopy=", setup_fail_usercopy);
> > +
> > +#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> > +
> > +static int __init fail_usercopy_debugfs(void)
> > +{
> > +       umode_t mode = S_IFREG | 0600;
> > +       struct dentry *dir;
> > +
> > +       dir = fault_create_debugfs_attr("fail_usercopy", NULL,
> > +                                       &fail_usercopy.attr);
> > +       if (IS_ERR(dir))
> > +               return PTR_ERR(dir);
> > +
> > +       debugfs_create_u32("failsize", mode, dir,
> > +                          &fail_usercopy.failsize);
> 
> Marco, what's the right way to annotate these concurrent accesses for KCSAN?

For debugfs variables that are accessed concurrently, the only
non-data-racy option (currently) is to use debugfs_create_atomic_t() and
make the variable an atomic_t.

If it's read-mostly as is the case here, and given that atomic_read() is
cheap (it maps to READ_ONCE on x86 and arm64), that'd be reasonable even
if performance is a concern.

> > +       return 0;
> > +}
> > +
> > +late_initcall(fail_usercopy_debugfs);
> > +
> > +#endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */
> > +
> > +/**
> > + * should_fail_usercopy() - Failure code or amount of bytes not to copy.
> > + * @n: Size of the original copy call.
> > + *
> > + * The general idea is to have a method which returns the amount of bytes not
> > + * to copy, a failure to return, or 0 if the calling function should progress
> > + * without a failure. E.g., copy_{to,from}_user should NOT copy the amount of
> > + * bytes returned by should_fail_usercopy, returning this value (in addition
> > + * to any bytes that could actually not be copied) or a failure.
> > + *
> > + * Return: one of:
> > + * negative, failure to return;
> > + * 0, progress normally;
> > + * a number in ]0, n], the number of bytes not to copy.
> > + *
> > + */
> > +long should_fail_usercopy(unsigned long n)
> > +{
> > +       if (should_fail(&fail_usercopy.attr, n)) {
> > +               if (fail_usercopy.failsize > 0)
> > +                       return fail_usercopy.failsize % (n + 1);

If you wanted to retain the u32 in debugfs, you can mark this
'data_race(fail_usercopy.failsize)' -- since what we're doing here is
probabilistic anyway, reading a garbage value won't affect things much.

Alternatively, just switch to atomic_t and it'll just be an
atomic_read().

Thanks,
-- Marco