linux-kernel - Re: [PATCH RESEND v3 2/3] kasan: migrate copy_user

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: <CA+fCnZf8YRH=gkmwU8enMLnGi7hHfVP4DSE2TLrmmVsHT10wRQ@mail.gmail.com>
Date: Wed, 16 Oct 2024 01:16:52 +0200
From: Andrey Konovalov <andreyknvl@...il.com>
To: Sabyrzhan Tasbolatov <snovitoll@...il.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, 2023002089@...k.tyut.edu.cn, 
	alexs@...nel.org, corbet@....net, dvyukov@...gle.com, elver@...gle.com, 
	glider@...gle.com, kasan-dev@...glegroups.com, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, ryabinin.a.a@...il.com, 
	siyanteng@...ngson.cn, vincenzo.frascino@....com, workflows@...r.kernel.org
Subject: Re: [PATCH RESEND v3 2/3] kasan: migrate copy_user_test to kunit

On Tue, Oct 15, 2024 at 12:52 PM Sabyrzhan Tasbolatov
<snovitoll@...il.com> wrote:
>
> > Too bad. I guess we have to duplicate both kasan_check_write and
> > check_object_size before both do_strncpy_from_user calls in
> > strncpy_from_user.
>
> Shall we do it once in strncpy_from_user() as I did in v1?
> Please let me know as I've tested in x86_64 and arm64 -
> there is no warning during kernel build with the diff below.
>
> These checks are for kernel pointer *dst only and size:
>    kasan_check_write(dst, count);
>    check_object_size(dst, count, false);
>
> And there are 2 calls of do_strncpy_from_user,
> which are implemented in x86 atm per commit 2865baf54077,
> and they are relevant to __user *src address, AFAIU.
>
> long strncpy_from_user()
>    if (can_do_masked_user_access()) {
>       src = masked_user_access_begin(src);
>       retval = do_strncpy_from_user(dst, src, count, count);
>       user_read_access_end();
>    }
>
>    if (likely(src_addr < max_addr)) {
>       if (user_read_access_begin(src, max)) {
>          retval = do_strncpy_from_user(dst, src, count, max);
>          user_read_access_end();
>
> ---
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 989a12a6787..6dc234913dd 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -120,6 +120,9 @@ long strncpy_from_user(char *dst, const char
> __user *src, long count)
>         if (unlikely(count <= 0))
>                 return 0;
>
> +       kasan_check_write(dst, count);
> +       check_object_size(dst, count, false);
> +
>         if (can_do_masked_user_access()) {
>                 long retval;
>
> @@ -142,8 +145,6 @@ long strncpy_from_user(char *dst, const char
> __user *src, long count)
>                 if (max > count)
>                         max = count;
>
> -               kasan_check_write(dst, count);
> -               check_object_size(dst, count, false);
>                 if (user_read_access_begin(src, max)) {
>                         retval = do_strncpy_from_user(dst, src, count, max);
>                         user_read_access_end();

Ok, let's do this. (What looked concerning to me with this approach
was doing the KASAN/userscopy checks outside of the src_addr <
max_addr, but I suppose that should be fine.)

Thank you!