linux-kernel - Re: [PATCH] mm, kmsan: instrument copy_from_kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANpmjNOZ4N5mhqWGvEU9zGBxj+jqhG3Q_eM1AbHp0cbSF=HqFw@mail.gmail.com>
Date: Sat, 5 Oct 2024 12:36:44 +0200
From: Marco Elver <elver@...gle.com>
To: Sabyrzhan Tasbolatov <snovitoll@...il.com>
Cc: ryabinin.a.a@...il.com, glider@...gle.com, andreyknvl@...il.com, 
	dvyukov@...gle.com, akpm@...ux-foundation.org, vincenzo.frascino@....com, 
	kasan-dev@...glegroups.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	bpf@...r.kernel.org, syzbot+61123a5daeb9f7454599@...kaller.appspotmail.com
Subject: Re: [PATCH] mm, kmsan: instrument copy_from_kernel_nofault

On Sat, 5 Oct 2024 at 11:22, Sabyrzhan Tasbolatov <snovitoll@...il.com> wrote:
>
> syzbot reported that bpf_probe_read_kernel() kernel helper triggered
> KASAN report via kasan_check_range() which is not the expected behaviour
> as copy_from_kernel_nofault() is meant to be a non-faulting helper.
>
> Solution is, suggested by Marco Elver, to replace KASAN, KCSAN check in
> copy_from_kernel_nofault() with KMSAN detection of copying uninitilaized
> kernel memory. In copy_to_kernel_nofault() we can retain
> instrument_write() for the memory corruption instrumentation but before
> pagefault_disable().
>
> Added KMSAN and modified KASAN kunit tests and tested on x86_64.
>
> This is the part of PATCH series attempting to properly address bugzilla
> issue.
>
> Link: https://lore.kernel.org/linux-mm/CANpmjNMAVFzqnCZhEity9cjiqQ9CVN1X7qeeeAp_6yKjwKo8iw@mail.gmail.com/
> Suggested-by: Marco Elver <elver@...gle.com>
> Reported-by: syzbot+61123a5daeb9f7454599@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=61123a5daeb9f7454599
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@...il.com>

I'm getting confused which parts are already picked up by Andrew into
-mm, and which aren't.

To clarify we have:
 1. https://lore.kernel.org/mm-commits/20240927171751.D1BD9C4CEC4@smtp.kernel.org/
 2. https://lore.kernel.org/mm-commits/20240930162435.9B6CBC4CED0@smtp.kernel.org/

And this is the 3rd patch, which applies on top of the other 2.

If my understanding is correct, rather than just adding fix on top of
fix, in the interest of having one clean patch which can also be
backported more easily, would it make sense to drop the first 2
patches from -mm, and you send out one clean patch series?

Thanks,
-- Marco

> ---
>  mm/kasan/kasan_test_c.c |  8 ++------
>  mm/kmsan/kmsan_test.c   | 17 +++++++++++++++++
>  mm/maccess.c            |  5 +++--
>  3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/mm/kasan/kasan_test_c.c b/mm/kasan/kasan_test_c.c
> index 0a226ab032d..5cff90f831d 100644
> --- a/mm/kasan/kasan_test_c.c
> +++ b/mm/kasan/kasan_test_c.c
> @@ -1954,7 +1954,7 @@ static void rust_uaf(struct kunit *test)
>         KUNIT_EXPECT_KASAN_FAIL(test, kasan_test_rust_uaf());
>  }
>
> -static void copy_from_to_kernel_nofault_oob(struct kunit *test)
> +static void copy_to_kernel_nofault_oob(struct kunit *test)
>  {
>         char *ptr;
>         char buf[128];
> @@ -1973,10 +1973,6 @@ static void copy_from_to_kernel_nofault_oob(struct kunit *test)
>                 KUNIT_EXPECT_LT(test, (u8)get_tag(ptr), (u8)KASAN_TAG_KERNEL);
>         }
>
> -       KUNIT_EXPECT_KASAN_FAIL(test,
> -               copy_from_kernel_nofault(&buf[0], ptr, size));
> -       KUNIT_EXPECT_KASAN_FAIL(test,
> -               copy_from_kernel_nofault(ptr, &buf[0], size));
>         KUNIT_EXPECT_KASAN_FAIL(test,
>                 copy_to_kernel_nofault(&buf[0], ptr, size));
>         KUNIT_EXPECT_KASAN_FAIL(test,
> @@ -2057,7 +2053,7 @@ static struct kunit_case kasan_kunit_test_cases[] = {
>         KUNIT_CASE(match_all_not_assigned),
>         KUNIT_CASE(match_all_ptr_tag),
>         KUNIT_CASE(match_all_mem_tag),
> -       KUNIT_CASE(copy_from_to_kernel_nofault_oob),
> +       KUNIT_CASE(copy_to_kernel_nofault_oob),
>         KUNIT_CASE(rust_uaf),
>         {}
>  };
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index 13236d579eb..9733a22c46c 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -640,6 +640,22 @@ static void test_unpoison_memory(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, report_matches(&expect));
>  }
>
> +static void test_copy_from_kernel_nofault(struct kunit *test)
> +{
> +       long ret;
> +       char buf[4], src[4];
> +       size_t size = sizeof(buf);
> +
> +       EXPECTATION_UNINIT_VALUE_FN(expect, "copy_from_kernel_nofault");
> +       kunit_info(
> +               test,
> +               "testing copy_from_kernel_nofault with uninitialized memory\n");
> +
> +       ret = copy_from_kernel_nofault((char *)&buf[0], (char *)&src[0], size);
> +       USE(ret);
> +       KUNIT_EXPECT_TRUE(test, report_matches(&expect));
> +}
> +
>  static struct kunit_case kmsan_test_cases[] = {
>         KUNIT_CASE(test_uninit_kmalloc),
>         KUNIT_CASE(test_init_kmalloc),
> @@ -664,6 +680,7 @@ static struct kunit_case kmsan_test_cases[] = {
>         KUNIT_CASE(test_long_origin_chain),
>         KUNIT_CASE(test_stackdepot_roundtrip),
>         KUNIT_CASE(test_unpoison_memory),
> +       KUNIT_CASE(test_copy_from_kernel_nofault),
>         {},
>  };
>
> diff --git a/mm/maccess.c b/mm/maccess.c
> index f752f0c0fa3..a91a39a56cf 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -31,8 +31,9 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>         if (!copy_from_kernel_nofault_allowed(src, size))
>                 return -ERANGE;
>
> +       /* Make sure uninitialized kernel memory isn't copied. */
> +       kmsan_check_memory(src, size);
>         pagefault_disable();
> -       instrument_read(src, size);
>         if (!(align & 7))
>                 copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
>         if (!(align & 3))
> @@ -63,8 +64,8 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
>         if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>                 align = (unsigned long)dst | (unsigned long)src;
>
> -       pagefault_disable();
>         instrument_write(dst, size);
> +       pagefault_disable();
>         if (!(align & 7))
>                 copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
>         if (!(align & 3))
> --
> 2.34.1
>