2022-07-01 14:39:52

by Alexander Potapenko

[permalink] [raw]
Subject: [PATCH v4 03/45] instrumented.h: allow instrumenting both sides of copy_from_user()

Introduce instrument_copy_from_user_before() and
instrument_copy_from_user_after() hooks to be invoked before and after
the call to copy_from_user().

KASAN and KCSAN will be only using instrument_copy_from_user_before(),
but for KMSAN we'll need to insert code after copy_from_user().

Signed-off-by: Alexander Potapenko <[email protected]>
---
v4:
-- fix _copy_from_user_key() in arch/s390/lib/uaccess.c (Reported-by:
kernel test robot <[email protected]>)

Link: https://linux-review.googlesource.com/id/I855034578f0b0f126734cbd734fb4ae1d3a6af99
---
arch/s390/lib/uaccess.c | 3 ++-
include/linux/instrumented.h | 21 +++++++++++++++++++--
include/linux/uaccess.h | 19 ++++++++++++++-----
lib/iov_iter.c | 9 ++++++---
lib/usercopy.c | 3 ++-
5 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index d7b3b193d1088..58033dfcb6d45 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -81,8 +81,9 @@ unsigned long _copy_from_user_key(void *to, const void __user *from,

might_fault();
if (!should_fail_usercopy()) {
- instrument_copy_from_user(to, from, n);
+ instrument_copy_from_user_before(to, from, n);
res = raw_copy_from_user_key(to, from, n, key);
+ instrument_copy_from_user_after(to, from, n, res);
}
if (unlikely(res))
memset(to + (n - res), 0, res);
diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
index 42faebbaa202a..ee8f7d17d34f5 100644
--- a/include/linux/instrumented.h
+++ b/include/linux/instrumented.h
@@ -120,7 +120,7 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
}

/**
- * instrument_copy_from_user - instrument writes of copy_from_user
+ * instrument_copy_from_user_before - add instrumentation before copy_from_user
*
* Instrument writes to kernel memory, that are due to copy_from_user (and
* variants). The instrumentation should be inserted before the accesses.
@@ -130,10 +130,27 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
* @n number of bytes to copy
*/
static __always_inline void
-instrument_copy_from_user(const void *to, const void __user *from, unsigned long n)
+instrument_copy_from_user_before(const void *to, const void __user *from, unsigned long n)
{
kasan_check_write(to, n);
kcsan_check_write(to, n);
}

+/**
+ * instrument_copy_from_user_after - add instrumentation after copy_from_user
+ *
+ * Instrument writes to kernel memory, that are due to copy_from_user (and
+ * variants). The instrumentation should be inserted after the accesses.
+ *
+ * @to destination address
+ * @from source address
+ * @n number of bytes to copy
+ * @left number of bytes not copied (as returned by copy_from_user)
+ */
+static __always_inline void
+instrument_copy_from_user_after(const void *to, const void __user *from,
+ unsigned long n, unsigned long left)
+{
+}
+
#endif /* _LINUX_INSTRUMENTED_H */
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 5a328cf02b75e..da16e96680cf1 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -58,20 +58,28 @@
static __always_inline __must_check unsigned long
__copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
{
- instrument_copy_from_user(to, from, n);
+ unsigned long res;
+
+ instrument_copy_from_user_before(to, from, n);
check_object_size(to, n, false);
- return raw_copy_from_user(to, from, n);
+ res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
+ return res;
}

static __always_inline __must_check unsigned long
__copy_from_user(void *to, const void __user *from, unsigned long n)
{
+ unsigned long res;
+
might_fault();
+ instrument_copy_from_user_before(to, from, n);
if (should_fail_usercopy())
return n;
- instrument_copy_from_user(to, from, n);
check_object_size(to, n, false);
- return raw_copy_from_user(to, from, n);
+ res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
+ return res;
}

/**
@@ -115,8 +123,9 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
unsigned long res = n;
might_fault();
if (!should_fail_usercopy() && likely(access_ok(from, n))) {
- instrument_copy_from_user(to, from, n);
+ instrument_copy_from_user_before(to, from, n);
res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
}
if (unlikely(res))
memset(to + (n - res), 0, res);
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 0b64695ab632f..fe5d169314dbf 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -159,13 +159,16 @@ static int copyout(void __user *to, const void *from, size_t n)

static int copyin(void *to, const void __user *from, size_t n)
{
+ size_t res = n;
+
if (should_fail_usercopy())
return n;
if (access_ok(from, n)) {
- instrument_copy_from_user(to, from, n);
- n = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_before(to, from, n);
+ res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
}
- return n;
+ return res;
}

static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t bytes,
diff --git a/lib/usercopy.c b/lib/usercopy.c
index 7413dd300516e..1505a52f23a01 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -12,8 +12,9 @@ unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n
unsigned long res = n;
might_fault();
if (!should_fail_usercopy() && likely(access_ok(from, n))) {
- instrument_copy_from_user(to, from, n);
+ instrument_copy_from_user_before(to, from, n);
res = raw_copy_from_user(to, from, n);
+ instrument_copy_from_user_after(to, from, n, res);
}
if (unlikely(res))
memset(to + (n - res), 0, res);
--
2.37.0.rc0.161.g10f37bed90-goog


2022-07-12 14:20:44

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v4 03/45] instrumented.h: allow instrumenting both sides of copy_from_user()

On Fri, 1 Jul 2022 at 16:23, Alexander Potapenko <[email protected]> wrote:
>
> Introduce instrument_copy_from_user_before() and
> instrument_copy_from_user_after() hooks to be invoked before and after
> the call to copy_from_user().
>
> KASAN and KCSAN will be only using instrument_copy_from_user_before(),
> but for KMSAN we'll need to insert code after copy_from_user().
>
> Signed-off-by: Alexander Potapenko <[email protected]>

Reviewed-by: Marco Elver <[email protected]>


> ---
> v4:
> -- fix _copy_from_user_key() in arch/s390/lib/uaccess.c (Reported-by:
> kernel test robot <[email protected]>)
>
> Link: https://linux-review.googlesource.com/id/I855034578f0b0f126734cbd734fb4ae1d3a6af99
> ---
> arch/s390/lib/uaccess.c | 3 ++-
> include/linux/instrumented.h | 21 +++++++++++++++++++--
> include/linux/uaccess.h | 19 ++++++++++++++-----
> lib/iov_iter.c | 9 ++++++---
> lib/usercopy.c | 3 ++-
> 5 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
> index d7b3b193d1088..58033dfcb6d45 100644
> --- a/arch/s390/lib/uaccess.c
> +++ b/arch/s390/lib/uaccess.c
> @@ -81,8 +81,9 @@ unsigned long _copy_from_user_key(void *to, const void __user *from,
>
> might_fault();
> if (!should_fail_usercopy()) {
> - instrument_copy_from_user(to, from, n);
> + instrument_copy_from_user_before(to, from, n);
> res = raw_copy_from_user_key(to, from, n, key);
> + instrument_copy_from_user_after(to, from, n, res);
> }
> if (unlikely(res))
> memset(to + (n - res), 0, res);
> diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> index 42faebbaa202a..ee8f7d17d34f5 100644
> --- a/include/linux/instrumented.h
> +++ b/include/linux/instrumented.h
> @@ -120,7 +120,7 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
> }
>
> /**
> - * instrument_copy_from_user - instrument writes of copy_from_user
> + * instrument_copy_from_user_before - add instrumentation before copy_from_user
> *
> * Instrument writes to kernel memory, that are due to copy_from_user (and
> * variants). The instrumentation should be inserted before the accesses.
> @@ -130,10 +130,27 @@ instrument_copy_to_user(void __user *to, const void *from, unsigned long n)
> * @n number of bytes to copy
> */
> static __always_inline void
> -instrument_copy_from_user(const void *to, const void __user *from, unsigned long n)
> +instrument_copy_from_user_before(const void *to, const void __user *from, unsigned long n)
> {
> kasan_check_write(to, n);
> kcsan_check_write(to, n);
> }
>
> +/**
> + * instrument_copy_from_user_after - add instrumentation after copy_from_user
> + *
> + * Instrument writes to kernel memory, that are due to copy_from_user (and
> + * variants). The instrumentation should be inserted after the accesses.
> + *
> + * @to destination address
> + * @from source address
> + * @n number of bytes to copy
> + * @left number of bytes not copied (as returned by copy_from_user)
> + */
> +static __always_inline void
> +instrument_copy_from_user_after(const void *to, const void __user *from,
> + unsigned long n, unsigned long left)
> +{
> +}
> +
> #endif /* _LINUX_INSTRUMENTED_H */
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 5a328cf02b75e..da16e96680cf1 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -58,20 +58,28 @@
> static __always_inline __must_check unsigned long
> __copy_from_user_inatomic(void *to, const void __user *from, unsigned long n)
> {
> - instrument_copy_from_user(to, from, n);
> + unsigned long res;
> +
> + instrument_copy_from_user_before(to, from, n);
> check_object_size(to, n, false);
> - return raw_copy_from_user(to, from, n);
> + res = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_after(to, from, n, res);
> + return res;
> }
>
> static __always_inline __must_check unsigned long
> __copy_from_user(void *to, const void __user *from, unsigned long n)
> {
> + unsigned long res;
> +
> might_fault();
> + instrument_copy_from_user_before(to, from, n);
> if (should_fail_usercopy())
> return n;
> - instrument_copy_from_user(to, from, n);
> check_object_size(to, n, false);
> - return raw_copy_from_user(to, from, n);
> + res = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_after(to, from, n, res);
> + return res;
> }
>
> /**
> @@ -115,8 +123,9 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
> unsigned long res = n;
> might_fault();
> if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> - instrument_copy_from_user(to, from, n);
> + instrument_copy_from_user_before(to, from, n);
> res = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_after(to, from, n, res);
> }
> if (unlikely(res))
> memset(to + (n - res), 0, res);
> diff --git a/lib/iov_iter.c b/lib/iov_iter.c
> index 0b64695ab632f..fe5d169314dbf 100644
> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -159,13 +159,16 @@ static int copyout(void __user *to, const void *from, size_t n)
>
> static int copyin(void *to, const void __user *from, size_t n)
> {
> + size_t res = n;
> +
> if (should_fail_usercopy())
> return n;
> if (access_ok(from, n)) {
> - instrument_copy_from_user(to, from, n);
> - n = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_before(to, from, n);
> + res = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_after(to, from, n, res);
> }
> - return n;
> + return res;
> }
>
> static size_t copy_page_to_iter_iovec(struct page *page, size_t offset, size_t bytes,
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index 7413dd300516e..1505a52f23a01 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -12,8 +12,9 @@ unsigned long _copy_from_user(void *to, const void __user *from, unsigned long n
> unsigned long res = n;
> might_fault();
> if (!should_fail_usercopy() && likely(access_ok(from, n))) {
> - instrument_copy_from_user(to, from, n);
> + instrument_copy_from_user_before(to, from, n);
> res = raw_copy_from_user(to, from, n);
> + instrument_copy_from_user_after(to, from, n, res);
> }
> if (unlikely(res))
> memset(to + (n - res), 0, res);
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>