[PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[Jann Horn]

In the optimized X86 version of the copy-with-checksum helpers, use
instrument_*() before accessing buffers from assembly code so that KASAN
and KCSAN don't have blind spots there.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/lib/csum-partial_64.c  | 3 +++
 arch/x86/lib/csum-wrappers_64.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/x86/lib/csum-partial_64.c b/arch/x86/lib/csum-partial_64.c
index 1f8a8f895173..8b0c353cd212 100644
--- a/arch/x86/lib/csum-partial_64.c
+++ b/arch/x86/lib/csum-partial_64.c
@@ -8,6 +8,7 @@
  
 #include <linux/compiler.h>
 #include <linux/export.h>
+#include <linux/instrumented.h>
 #include <asm/checksum.h>
 #include <asm/word-at-a-time.h>
 
@@ -37,6 +38,8 @@ __wsum csum_partial(const void *buff, int len, __wsum sum)
 	u64 temp64 = (__force u64)sum;
 	unsigned odd, result;
 
+	instrument_read(buff, len);
+
 	odd = 1 & (unsigned long) buff;
 	if (unlikely(odd)) {
 		if (unlikely(len == 0))
diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c
index 189344924a2b..087f3c4cb89f 100644
--- a/arch/x86/lib/csum-wrappers_64.c
+++ b/arch/x86/lib/csum-wrappers_64.c
@@ -6,6 +6,8 @@
  */
 #include <asm/checksum.h>
 #include <linux/export.h>
+#include <linux/in6.h>
+#include <linux/instrumented.h>
 #include <linux/uaccess.h>
 #include <asm/smap.h>
 
@@ -26,6 +28,7 @@ csum_and_copy_from_user(const void __user *src, void *dst, int len)
 	__wsum sum;
 
 	might_sleep();
+	instrument_write(dst, len);
 	if (!user_access_begin(src, len))
 		return 0;
 	sum = csum_partial_copy_generic((__force const void *)src, dst, len);
@@ -51,6 +54,7 @@ csum_and_copy_to_user(const void *src, void __user *dst, int len)
 	__wsum sum;
 
 	might_sleep();
+	instrument_read(src, len);
 	if (!user_access_begin(dst, len))
 		return 0;
 	sum = csum_partial_copy_generic(src, (void __force *)dst, len);
@@ -71,6 +75,8 @@ EXPORT_SYMBOL(csum_and_copy_to_user);
 __wsum
 csum_partial_copy_nocheck(const void *src, void *dst, int len)
 {
+	instrument_write(dst, len);
+	instrument_read(src, len);
 	return csum_partial_copy_generic(src, dst, len);
 }
 EXPORT_SYMBOL(csum_partial_copy_nocheck);
@@ -81,6 +87,9 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
 {
 	__u64 rest, sum64;
 
+	instrument_read(saddr, sizeof(*saddr));
+	instrument_read(daddr, sizeof(*daddr));
+
 	rest = (__force __u64)htonl(len) + (__force __u64)htons(proto) +
 		(__force __u64)sum;
 

base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
-- 
2.35.0.rc0.227.g00780c9af4-goog

Re: [PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[Dmitry Vyukov]

On Wed, 26 Jan 2022 at 18:13, Jann Horn <jannh@google.com> wrote:
>
> In the optimized X86 version of the copy-with-checksum helpers, use
> instrument_*() before accessing buffers from assembly code so that KASAN
> and KCSAN don't have blind spots there.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  arch/x86/lib/csum-partial_64.c  | 3 +++
>  arch/x86/lib/csum-wrappers_64.c | 9 +++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/arch/x86/lib/csum-partial_64.c b/arch/x86/lib/csum-partial_64.c
> index 1f8a8f895173..8b0c353cd212 100644
> --- a/arch/x86/lib/csum-partial_64.c
> +++ b/arch/x86/lib/csum-partial_64.c
> @@ -8,6 +8,7 @@
>
>  #include <linux/compiler.h>
>  #include <linux/export.h>
> +#include <linux/instrumented.h>
>  #include <asm/checksum.h>
>  #include <asm/word-at-a-time.h>
>
> @@ -37,6 +38,8 @@ __wsum csum_partial(const void *buff, int len, __wsum sum)
>         u64 temp64 = (__force u64)sum;
>         unsigned odd, result;
>
> +       instrument_read(buff, len);
> +
>         odd = 1 & (unsigned long) buff;
>         if (unlikely(odd)) {
>                 if (unlikely(len == 0))
> diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c
> index 189344924a2b..087f3c4cb89f 100644
> --- a/arch/x86/lib/csum-wrappers_64.c
> +++ b/arch/x86/lib/csum-wrappers_64.c
> @@ -6,6 +6,8 @@
>   */
>  #include <asm/checksum.h>
>  #include <linux/export.h>
> +#include <linux/in6.h>
> +#include <linux/instrumented.h>
>  #include <linux/uaccess.h>
>  #include <asm/smap.h>
>
> @@ -26,6 +28,7 @@ csum_and_copy_from_user(const void __user *src, void *dst, int len)
>         __wsum sum;
>
>         might_sleep();
> +       instrument_write(dst, len);
>         if (!user_access_begin(src, len))
>                 return 0;
>         sum = csum_partial_copy_generic((__force const void *)src, dst, len);
> @@ -51,6 +54,7 @@ csum_and_copy_to_user(const void *src, void __user *dst, int len)
>         __wsum sum;
>
>         might_sleep();
> +       instrument_read(src, len);

Nice!

Can these potentially be called with KERNEL_DS as in some compat
syscalls? If so it's better to use instrument_copy_to/from_user.
Or probably it's better to use them anyway b/c we also want to know
about user accesses for uaccess logging and maybe other things.



>         if (!user_access_begin(dst, len))
>                 return 0;
>         sum = csum_partial_copy_generic(src, (void __force *)dst, len);
> @@ -71,6 +75,8 @@ EXPORT_SYMBOL(csum_and_copy_to_user);
>  __wsum
>  csum_partial_copy_nocheck(const void *src, void *dst, int len)
>  {
> +       instrument_write(dst, len);
> +       instrument_read(src, len);
>         return csum_partial_copy_generic(src, dst, len);
>  }
>  EXPORT_SYMBOL(csum_partial_copy_nocheck);
> @@ -81,6 +87,9 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
>  {
>         __u64 rest, sum64;
>
> +       instrument_read(saddr, sizeof(*saddr));
> +       instrument_read(daddr, sizeof(*daddr));
> +
>         rest = (__force __u64)htonl(len) + (__force __u64)htons(proto) +
>                 (__force __u64)sum;
>
>
> base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

Re: [PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[Jann Horn]

On Wed, Jan 26, 2022 at 6:38 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, 26 Jan 2022 at 18:13, Jann Horn <jannh@google.com> wrote:
> >
> > In the optimized X86 version of the copy-with-checksum helpers, use
> > instrument_*() before accessing buffers from assembly code so that KASAN
> > and KCSAN don't have blind spots there.
[...]
> Can these potentially be called with KERNEL_DS as in some compat
> syscalls? If so it's better to use instrument_copy_to/from_user.
> Or probably it's better to use them anyway b/c we also want to know
> about user accesses for uaccess logging and maybe other things.

Christoph Hellwig has basically eradicated KERNEL_DS. :)

In particular, on X86, set_fs(KERNEL_DS) doesn't really do anything
anymore since commit 47058bb54b579 ("x86: remove address space
overrides using set_fs()").

Re: [PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[Dmitry Vyukov]

On Wed, 26 Jan 2022 at 18:48, Jann Horn <jannh@google.com> wrote:
> > > In the optimized X86 version of the copy-with-checksum helpers, use
> > > instrument_*() before accessing buffers from assembly code so that KASAN
> > > and KCSAN don't have blind spots there.
> [...]
> > Can these potentially be called with KERNEL_DS as in some compat
> > syscalls? If so it's better to use instrument_copy_to/from_user.
> > Or probably it's better to use them anyway b/c we also want to know
> > about user accesses for uaccess logging and maybe other things.
>
> Christoph Hellwig has basically eradicated KERNEL_DS. :)
>
> In particular, on X86, set_fs(KERNEL_DS) doesn't really do anything
> anymore since commit 47058bb54b579 ("x86: remove address space
> overrides using set_fs()").

That's good!

But still need this for uaccess logging.

Re: [PATCH] x86/csum: Add KASAN/KCSAN instrumentation

[Andrey Konovalov]

On Wed, Jan 26, 2022 at 6:13 PM Jann Horn <jannh@google.com> wrote:
>
> In the optimized X86 version of the copy-with-checksum helpers, use
> instrument_*() before accessing buffers from assembly code so that KASAN
> and KCSAN don't have blind spots there.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  arch/x86/lib/csum-partial_64.c  | 3 +++
>  arch/x86/lib/csum-wrappers_64.c | 9 +++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/arch/x86/lib/csum-partial_64.c b/arch/x86/lib/csum-partial_64.c
> index 1f8a8f895173..8b0c353cd212 100644
> --- a/arch/x86/lib/csum-partial_64.c
> +++ b/arch/x86/lib/csum-partial_64.c
> @@ -8,6 +8,7 @@
>
>  #include <linux/compiler.h>
>  #include <linux/export.h>
> +#include <linux/instrumented.h>
>  #include <asm/checksum.h>
>  #include <asm/word-at-a-time.h>
>
> @@ -37,6 +38,8 @@ __wsum csum_partial(const void *buff, int len, __wsum sum)
>         u64 temp64 = (__force u64)sum;
>         unsigned odd, result;
>
> +       instrument_read(buff, len);
> +
>         odd = 1 & (unsigned long) buff;
>         if (unlikely(odd)) {
>                 if (unlikely(len == 0))
> diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c
> index 189344924a2b..087f3c4cb89f 100644
> --- a/arch/x86/lib/csum-wrappers_64.c
> +++ b/arch/x86/lib/csum-wrappers_64.c
> @@ -6,6 +6,8 @@
>   */
>  #include <asm/checksum.h>
>  #include <linux/export.h>
> +#include <linux/in6.h>
> +#include <linux/instrumented.h>
>  #include <linux/uaccess.h>
>  #include <asm/smap.h>
>
> @@ -26,6 +28,7 @@ csum_and_copy_from_user(const void __user *src, void *dst, int len)
>         __wsum sum;
>
>         might_sleep();
> +       instrument_write(dst, len);
>         if (!user_access_begin(src, len))
>                 return 0;
>         sum = csum_partial_copy_generic((__force const void *)src, dst, len);
> @@ -51,6 +54,7 @@ csum_and_copy_to_user(const void *src, void __user *dst, int len)
>         __wsum sum;
>
>         might_sleep();
> +       instrument_read(src, len);
>         if (!user_access_begin(dst, len))
>                 return 0;
>         sum = csum_partial_copy_generic(src, (void __force *)dst, len);
> @@ -71,6 +75,8 @@ EXPORT_SYMBOL(csum_and_copy_to_user);
>  __wsum
>  csum_partial_copy_nocheck(const void *src, void *dst, int len)
>  {
> +       instrument_write(dst, len);
> +       instrument_read(src, len);
>         return csum_partial_copy_generic(src, dst, len);
>  }
>  EXPORT_SYMBOL(csum_partial_copy_nocheck);
> @@ -81,6 +87,9 @@ __sum16 csum_ipv6_magic(const struct in6_addr *saddr,
>  {
>         __u64 rest, sum64;
>
> +       instrument_read(saddr, sizeof(*saddr));
> +       instrument_read(daddr, sizeof(*daddr));
> +
>         rest = (__force __u64)htonl(len) + (__force __u64)htons(proto) +
>                 (__force __u64)sum;
>
>
> base-commit: 0280e3c58f92b2fe0e8fbbdf8d386449168de4a8
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

Acked-by: Andrey Konovalov <andreyknvl@gmail.com>

It would also be nice to add tests to check these, but since we still
don't have tests that check atomics [1], csum tests can be added
together with them once someone gets to doing that.

Thanks!

[1] https://bugzilla.kernel.org/show_bug.cgi?id=214055