From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B321CFA774 for ; Fri, 4 Oct 2024 12:37:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1E9656B0400; Fri, 4 Oct 2024 08:37:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 199726B0401; Fri, 4 Oct 2024 08:37:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 089096B0402; Fri, 4 Oct 2024 08:37:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id DF6DD6B0400 for ; Fri, 4 Oct 2024 08:37:05 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 8BC7281859 for ; Fri, 4 Oct 2024 12:37:05 +0000 (UTC) X-FDA: 82635869610.17.3A6666D Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf16.hostedemail.com (Postfix) with ESMTP id ABB25180008 for ; Fri, 4 Oct 2024 12:37:03 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=W9V8DlI+; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf16.hostedemail.com: domain of snovitoll@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=snovitoll@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1728045327; a=rsa-sha256; cv=none; b=BgsJJIWo9Vv9YOCoJrkqkmM8Wy+EwKzxMp2tBMYJmw8d/f8bRUJ/DTd9BnpGnciFzagxIn dXP1+F2jBlJP5C5XUaHR0xtqDHdPv3sKuA5+AeKVwSZuPsco0worby51yn6bm6/sYgJpg6 xgChs/YHpqtHYt/0fEQZwMfmqMb/dJo= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=W9V8DlI+; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf16.hostedemail.com: domain of snovitoll@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=snovitoll@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1728045327; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zkQGIL5EVYPHu1HndHhTEW+Z6R2TaJeNi0ejVqgiqaI=; b=0MFdoLbKM7xqfmeIfRa2HFipdphxOHY2hawHBGM+gefgp0WnAI84a0GFWm97sxShroG+df 2rdh0Vvg7kvRBE34g8jBEsatnyfz+kEnWhTFlmx7CaZu1uPziW35ZYbty4g3tiFZODJVT2 JcjvOE5xv/VQ4JnFloKAf+l5/qOzBH4= Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-5c42e7adbddso2374916a12.2 for ; Fri, 04 Oct 2024 05:37:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1728045422; x=1728650222; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=zkQGIL5EVYPHu1HndHhTEW+Z6R2TaJeNi0ejVqgiqaI=; b=W9V8DlI+fxtb0cpcmKGWPjPG48S7gsXyMSFMznmwReFpRwgsB3zwOdeFkpM4Pp8rul gA9OPwUCW/0B5Dfn3g8c/bWrvbOChiDlvK6Ndw8MyTdKAWillu8MQ/VabneGtZF/g+OM UAfoG/1NqejgWb8J3/NWoXfbAlgEW44C3L5//lIwwwuiWpAp44ibWNv1+Xm4XH5Vju+L f9BbXNx/o2zFmGnCJvjzspYbWFQWrLeWcmfK77Ah3JOXny0/mwQntXzG8fs4eNBlrntg 8Dr0cSCtQTsEBNqKIENukMn1chqZIZ1MDEW6tdnrWmO/J7WTDo8E4MJjgCSxKPIPhLDM DZtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728045422; x=1728650222; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zkQGIL5EVYPHu1HndHhTEW+Z6R2TaJeNi0ejVqgiqaI=; b=k43z+ZH+NxMVy50kQUES43XtaBDcaZ03RTnvA2ivsCuQfpzrq4+tS6495P6FIRlPz6 RUY6jSN1ecoZnEWzzXaDCczHjdrTNL/Kqs5aV5xQku0jZceJYu7JB29cTbpHBPL5GDwK nS6ZwKzpV5m0xIiyulD88K/EK761xDGkvXHtPYDJThs9PyxgxYJwdzIX0leemsaUsUvD kVFJahuJT/jJwTEhed5l1i1T1VnuBSI1F5kwWg1WOREbLExWBuWLJhwhJMM8VXSsBtaL jD27vIhvSmIH+IPokkUaQm0te3XIMTRFUozQF0muZfBetSHyKAfKnnGdfHytb4LsvpQw Hj6Q== X-Forwarded-Encrypted: i=1; AJvYcCWrDxYJ132SxyNt9FKW77xAD20/QUxs3gRzNT2DOZ5LIotSy1KMlOGmiyMAyr6FYH3SQD+abmfJKQ==@kvack.org X-Gm-Message-State: AOJu0YwfgJORJutYCA2jCzxKNwr9BkqL68bMEANL8+Iet30tvFrcfqHg chvCFYniCIGGuE3BOAFK1hLUSwLlqs4ojg8YQW55bZWWEBJ4Qf7XYrKZB9JN041i2Rbkr7zdUsU tAu/hW1iMjEuoBxMxg8YCGKTlc4I= X-Google-Smtp-Source: AGHT+IFgvoRyJjs0lIyE4qY6H9T2dmOzQx1UEDkQrUquZSvU+x5YvpAG94POLKxLmfdFd8jtUSOHLrY8dnKcsjlOJ54= X-Received: by 2002:a05:6402:348e:b0:5c8:9696:bae8 with SMTP id 4fb4d7f45d1cf-5c8d2ed2ed0mr1700748a12.32.1728045421608; Fri, 04 Oct 2024 05:37:01 -0700 (PDT) MIME-Version: 1.0 References: <20240927151438.2143936-1-snovitoll@gmail.com> In-Reply-To: From: Sabyrzhan Tasbolatov Date: Fri, 4 Oct 2024 17:37:45 +0500 Message-ID: Subject: Re: [PATCH] mm: instrument copy_from/to_kernel_nofault To: Marco Elver Cc: ryabinin.a.a@gmail.com, glider@google.com, andreyknvl@gmail.com, dvyukov@google.com, vincenzo.frascino@arm.com, akpm@linux-foundation.org, kasan-dev@googlegroups.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: ABB25180008 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 1hj61sw7cruw1f6pgjgycafecmxhmgka X-HE-Tag: 1728045423-364583 X-HE-Meta: U2FsdGVkX19+niXcvluVw/ChcpF5bMrs78vNxDCfpgNWxUKCmw3rPIKSD97JGA1HNJBBd+DHbvzIRRZ+oFb2e2vLL1XZNam6dEqh0ojx0FUhpqvNxbXYpf0fIeNTTgP/nqWMzDocZCCWub45rXCwLqOZ/T9ro3cT9CEgUCGkvluMGnROFo+Nhx1NXlax0DKC4jDHdWjUjFlAs/sjjlNUx+y1iYkQnifJd8xzj4wEjxEiMC0RFHKpmTqvkBz9BjlbT6f7co++RFQSTuPBbwQx0DhbIgSWH2z5zFS2oUrtYNeaEsWQROZxnWgpJKePg4QMFgmgB+/+ahvtkyU7jF227BgTqxkTP3YFAlHTdQ7HuBBpctlD1cBq7sGWrpw0VUYspFfSYm9oerpNwBhaoWlgxA6iSNVv/mUkojLcg8D4b2BwjERvZZouEpg2n/T1O6+G5gfRqrY2Q7bRCOAjyw0thASKWAhbTG0r40Me7k4IYeq1KutDdttVIBwZV/JN8ytz4+Nu7mdmHtdWrz2UI9CCLDv2ftvjeF9e172dsX2/FVSvwd4MPEMwuJ54qyXaxU8rYPbgSHnzWVFee31xigfs8ftSITCTi61d2CCAuiWxKUb3vCReb22LjeOfGJoIQNxBoPDnnWryeJ4ml0251l/+8WqZ/phTLrH+KFI75YoiWB0Sl/3Aju6XqsnQchdeMbxwi5Hd27t6e3f6KENw0vrr42iOI3Ea7WumanvSsqfnapX0iahxBj7pHEfDyQUmVUjPWeCfXAaN4JvzcSBEmnQEr4ZmhfNKxWlckfeMsVDps3eVkbya3eLHuPKiXxfIlQuoCvE+y6L8L1Dgpv1jmYGvDIF/T25yLaehT35Uy+hR6tUmsScE/M0Za5dYuS5BUdtEEKEaBBqsuhfko5HGVpyr+2DwHUzdtJJMWrJtUqNMgTVjMoamGaxQZ04tOFADqLJ/cRIiEfA/FHfU4WNLCTs iwi2rnTq 0sMANKHCA3Tp4DFu3sOrvc2IKDgTu6ELVWuGXBZ8h9n7uKv4ATA8fCoobT8UOe5Gv9ORqUeP4Xyw1PROfDLYLGvAlbPv3nCbRGecZuRIR+gCcnogJpPm4vTCItE0r1nSc3luiZOc99Wt8+5y1cHKdfJf1YWBbGUOPuc0+Sk9rdgCMSdaedTd7c4VnO4/L5dCUO0INJugnBMzEDTu86iliPvpYCqrLIpWcttHsca2yphBgtYCYZa2mMRtsjhHRPcXhe0IW81GbzCT3hpOjLGKNXrp1iCNfdidwYmGu/l4Oz83MVSbTP6l0C1a3tFCIWm75HMVI0zChW3pEnFLWXMP0xp+2rrm5HMSqE9MACBpGpfpVPa6nO3E9qUgFFAR+bxSTDl7+gwP74HFFTMTgSBkfijuRrDudghPgswGc+jJEmkoI1hr+C4BtRAO6orthPt0/mtT0h7KOlTu+atMi9qbyBTSrjDi+8c0NA0ycuUsMOR9UUj8= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Oct 4, 2024 at 11:55=E2=80=AFAM Marco Elver wrot= e: > > On Wed, 2 Oct 2024 at 18:40, Sabyrzhan Tasbolatov w= rote: > > > > On Wed, Oct 2, 2024 at 9:00=E2=80=AFPM Marco Elver w= rote: > > > > > > On Fri, 27 Sept 2024 at 17:14, Sabyrzhan Tasbolatov wrote: > > > > > > > > Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault() > > > > with instrument_memcpy_before() for KASAN, KCSAN checks and > > > > instrument_memcpy_after() for KMSAN. > > > > > > There's a fundamental problem with instrumenting > > > copy_from_kernel_nofault() - it's meant to be a non-faulting helper, > > > i.e. if it attempts to read arbitrary kernel addresses, that's not a > > > problem because it won't fault and BUG. These may be used in places > > > that probe random memory, and KASAN may say that some memory is > > > invalid and generate a report - but in reality that's not a problem. > > > > > > In the Bugzilla bug, Andrey wrote: > > > > > > > KASAN should check both arguments of copy_from/to_kernel_nofault() = for accessibility when both are fault-safe. > > > > > > I don't see this patch doing it, or at least it's not explained. By > > > looking at the code, I see that it does the instrument_memcpy_before(= ) > > > right after pagefault_disable(), which tells me that KASAN or other > > > tools will complain if a page is not faulted in. These helpers are > > > meant to be usable like that - despite their inherent unsafety, > > > there's little that I see that KASAN can help with. > > > > Hello, thanks for the comment! > > instrument_memcpy_before() has been replaced with > > instrument_read() and instrument_write() in > > commit 9e3f2b1ecdd4("mm, kasan: proper instrument _kernel_nofault"), > > and there are KASAN, KCSAN checks. > > > > > What _might_ be useful, is detecting copying faulted-in but > > > uninitialized memory to user space. So I think the only > > > instrumentation we want to retain is KMSAN instrumentation for the > > > copy_from_kernel_nofault() helper, and only if no fault was > > > encountered. > > > > > > Instrumenting copy_to_kernel_nofault() may be helpful to catch memory > > > corruptions, but only if faulted-in memory was accessed. > > > > If we need to have KMSAN only instrumentation for > > copy_from_user_nofault(), then AFAIU, in mm/kasan/kasan_test.c > > Did you mean s/copy_from_user_nofault/copy_from_kernel_nofault/? Yes, typo, sorry for the confusion. > > > copy_from_to_kernel_nofault_oob() should have only > > copy_to_kernel_nofault() OOB kunit test to trigger KASAN. > > And copy_from_user_nofault() kunit test can be placed in mm/kmsan/kmsan= _test.c. > > I think in the interest of reducing false positives, I'd proceed with > making copy_from_kernel_nofault() KMSAN only. Here is my current upcoming patch that I will send separately once it's tested, it's slowly being compiled on my laptop (away from PC). I've moved copy_from_kernel_nofault() to kmsan_test.c and added kmsan_check_memory() _before_ pagefault_disable() to check kernel src address if it's initialized. For copy_to_kernel_nofault() , I've left instrument_write() for memory corruption check but before pagefault_disable() again, if I understood the = logic correctly. I will adjust kmsan kunit test once I can run it and send a PATC= H. Meanwhile, please let me know if the order of instrumentation before pagefault_disable() is correct. > By looking at the code, I see that it does the instrument_memcpy_before() > right after pagefault_disable(), which tells me that KASAN or other > tools will complain if a page is not faulted in. These helpers are > meant to be usable like that - despite their inherent unsafety, > there's little that I see that KASAN can help with. --- mm/kasan/kasan_test_c.c | 8 ++------ mm/kmsan/kmsan_test.c | 16 ++++++++++++++++ mm/maccess.c | 5 +++-- 3 files changed, 21 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[] =3D= { 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..fc50d0aef47 100644 --- a/mm/kmsan/kmsan_test.c +++ b/mm/kmsan/kmsan_test.c @@ -640,6 +640,21 @@ 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; + volatile char src[4], dst[4]; + + EXPECTATION_UNINIT_VALUE_FN(expect, "test_copy_from_kernel_nofault"); + kunit_info( + test, + "testing copy_from_kernel_nofault with src uninitialized memory\n"); + + ret =3D copy_from_kernel_nofault(dst, src, sizeof(src)); + USE(ret); + KUNIT_EXPECT_TRUE(test, report_matches(&expect)); +} + static struct kunit_case kmsan_test_cases[] =3D { KUNIT_CASE(test_uninit_kmalloc), KUNIT_CASE(test_init_kmalloc), @@ -664,6 +679,7 @@ static struct kunit_case kmsan_test_cases[] =3D { 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 =3D (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)) --=20 2.34.1