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

From: Dmitry Vyukov
Date: Fri Aug 21 2020 - 07:51:27 EST

Next message: kernel test robot: "Re: [PATCH v5 8/8] mm/vmalloc: Hugepage vmalloc mappings"
Previous message: Wan Ahmad Zainie: "[PATCH v7 0/3] phy: intel: Add Keem Bay eMMC PHY support"
In reply to: albert . linde: "[PATCH 2/3] lib, uaccess: add failure injection to usercopy functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Aug 21, 2020 at 12:50 PM <albert.linde@xxxxxxxxx> wrote:
>
> From: Albert van der Linde <alinde@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>

Next message: kernel test robot: "Re: [PATCH v5 8/8] mm/vmalloc: Hugepage vmalloc mappings"
Previous message: Wan Ahmad Zainie: "[PATCH v7 0/3] phy: intel: Add Keem Bay eMMC PHY support"
In reply to: albert . linde: "[PATCH 2/3] lib, uaccess: add failure injection to usercopy functions"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]