arch/x86/include/asm/uaccess.h | 4 ++++ mm/kasan/kasan_test.c | 17 +++++++++++++++++ 2 files changed, 21 insertions(+)
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
strncpy_from_kernel_nofault() where __put_kernel_nofault,
__get_kernel_nofault macros are used.
Regular instrument_read() and instrument_write() handles KASAN, KCSAN
checks for the access address, though instrument_memcpy_before() might
be considered as well for both src and dst address validation.
__get_user_size was appended with instrument_get_user() for KMSAN check in
commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
arch/x86/include/asm/uaccess.h | 4 ++++
mm/kasan/kasan_test.c | 17 +++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..bed84d3f7245 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -353,6 +353,7 @@ do { \
default: \
(x) = __get_user_bad(); \
} \
+ instrument_get_user(x); \
} while (0)
#define __get_user_asm(x, addr, err, itype) \
@@ -620,6 +621,7 @@ do { \
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __get_kernel_nofault(dst, src, type, err_label) \
+ instrument_read(src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), err_label)
#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
@@ -627,6 +629,7 @@ do { \
do { \
int __kr_err; \
\
+ instrument_read(src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), __kr_err); \
if (unlikely(__kr_err)) \
@@ -635,6 +638,7 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __put_kernel_nofault(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 7b32be2a3cf0..f5086c86e0bd 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1899,6 +1899,22 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault(struct kunit *test)
+{
+ char *ptr;
+ char buf[16];
+ size_t size = sizeof(buf);
+
+ ptr = kmalloc(size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+ kfree(ptr);
+
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_from_kernel_nofault(&buf[0], ptr, size));
+ KUNIT_EXPECT_KASAN_FAIL(test,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -1971,6 +1987,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),
{}
};
--
2.34.1On Tue, Sep 17, 2024 at 10:18 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
> strncpy_from_kernel_nofault() where __put_kernel_nofault,
> __get_kernel_nofault macros are used.
>
> Regular instrument_read() and instrument_write() handles KASAN, KCSAN
> checks for the access address, though instrument_memcpy_before() might
> be considered as well for both src and dst address validation.
>
> __get_user_size was appended with instrument_get_user() for KMSAN check in
> commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
> put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
>
> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
Hi Sabyrzhan,
Thanks for working on this!
> diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
> index 7b32be2a3cf0..f5086c86e0bd 100644
> --- a/mm/kasan/kasan_test.c
> +++ b/mm/kasan/kasan_test.c
> @@ -1899,6 +1899,22 @@ static void match_all_mem_tag(struct kunit *test)
> kfree(ptr);
> }
>
> +static void copy_from_to_kernel_nofault(struct kunit *test)
> +{
> + char *ptr;
> + char buf[16];
> + size_t size = sizeof(buf);
> +
> + ptr = kmalloc(size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> + kfree(ptr);
> +
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_from_kernel_nofault(&buf[0], ptr, size));
> + KUNIT_EXPECT_KASAN_FAIL(test,
> + copy_to_kernel_nofault(ptr, &buf[0], size));
I just realized that the test I wrote in the bug report is not good.
This call will overwrite the object's contents and thus corrupt the
freelist pointer. This might cause crashes in further tests. KASAN
tests try to avoid harmfully corrupting memory to avoid crashes.
I think the easiest fix would be to allocate e.g. 128 -
KASAN_GRANULE_SIZE bytes and do an out-of-bounds up to 128 bytes via
copy_to/from_kernel_nofault. This will only corrupt the in-object
kmalloc redzone, which is not harmful.
Also, I think we need to test all 4 calls that I had in the bug report
to check both arguments of both functions. Not only the 2 you
included.
> +}
Thank you!
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
strncpy_from_kernel_nofault() where __put_kernel_nofault, __get_kernel_nofault
macros are used.
__get_kernel_nofault needs instrument_memcpy_before() which handles
KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
macro, instrument_write() check should be enough as it's validated via
kmsan_copy_to_user() in instrument_put_user().
__get_user_size was appended with instrument_get_user() for KMSAN check in
commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
copy_from_to_kernel_nofault() kunit test triggers 4 KASAN bug reports as
expected per each copy_from/to_kernel_nofault call.
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
v2: added 2 tests for src, dst check and enhanced instrument check in macros
On Wed, Sep 18, 2024 at 3:51 AM Andrey Konovalov <andreyknvl@gmail.com> wrote:
>
> On Tue, Sep 17, 2024 at 10:18 PM Sabyrzhan Tasbolatov
> <snovitoll@gmail.com> wrote:
> >
> I think the easiest fix would be to allocate e.g. 128 -
> KASAN_GRANULE_SIZE bytes and do an out-of-bounds up to 128 bytes via
> copy_to/from_kernel_nofault. This will only corrupt the in-object
> kmalloc redzone, which is not harmful.
Hi Andrey,
Thanks for the comments. I've changed the target UAF buffer size to
KASAN_GRANULE_SIZE and added other test cases from bugzilla.
---
arch/x86/include/asm/uaccess.h | 4 ++++
mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..bd1ee79238a2 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -353,6 +353,7 @@ do { \
default: \
(x) = __get_user_bad(); \
} \
+ instrument_get_user(x); \
} while (0)
#define __get_user_asm(x, addr, err, itype) \
@@ -620,6 +621,7 @@ do { \
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __get_kernel_nofault(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), err_label)
#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
@@ -627,6 +629,7 @@ do { \
do { \
int __kr_err; \
\
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), __kr_err); \
if (unlikely(__kr_err)) \
@@ -635,6 +638,7 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __put_kernel_nofault(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 7b32be2a3cf0..9a3c4ad91d59 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault(struct kunit *test)
+{
+ char *ptr;
+ char buf[KASAN_GRANULE_SIZE];
+ size_t size = sizeof(buf);
+
+ ptr = kmalloc(size, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+ kfree(ptr);
+
+ 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,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -1971,6 +1991,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),
{}
};
--
2.34.1
On Wed, Sep 18, 2024 at 12:57 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
> index 7b32be2a3cf0..9a3c4ad91d59 100644
> --- a/mm/kasan/kasan_test.c
> +++ b/mm/kasan/kasan_test.c
> @@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test)
> kfree(ptr);
> }
>
> +static void copy_from_to_kernel_nofault(struct kunit *test)
> +{
> + char *ptr;
> + char buf[KASAN_GRANULE_SIZE];
> + size_t size = sizeof(buf);
> +
> + ptr = kmalloc(size, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> + kfree(ptr);
> +
> + 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,
> + copy_to_kernel_nofault(ptr, &buf[0], size));
> +}
You still have the same problem here.
What I meant is:
char *ptr;
char buf[128 - KASAN_GRANULE_SIZE];
size_t size = sizeof(buf);
ptr = kmalloc(size, GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
KUNIT_EXPECT_KASAN_FAIL(...);
...
kfree(ptr);
Thanks!
On Wed, Sep 18, 2024 at 8:15 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> You still have the same problem here.
>
> What I meant is:
>
> char *ptr;
> char buf[128 - KASAN_GRANULE_SIZE];
> size_t size = sizeof(buf);
>
> ptr = kmalloc(size, GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
>
> KUNIT_EXPECT_KASAN_FAIL(...);
> ...
>
> kfree(ptr);
Thanks for catching this! I've turned kunit test into OOB instead of UAF.
---
v3: changed kunit test from UAF to OOB case and git commit message.
---
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
strncpy_from_kernel_nofault() where __put_kernel_nofault, __get_kernel_nofault
macros are used.
__get_kernel_nofault needs instrument_memcpy_before() which handles
KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
macro, instrument_write() check should be enough as it's validated via
kmsan_copy_to_user() in instrument_put_user().
__get_user_size was appended with instrument_get_user() for KMSAN check in
commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB bug reports
as expected for each copy_from/to_kernel_nofault call.
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
arch/x86/include/asm/uaccess.h | 4 ++++
mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..87fb59071e8c 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -353,6 +353,7 @@ do { \
default: \
(x) = __get_user_bad(); \
} \
+ instrument_get_user(x); \
} while (0)
#define __get_user_asm(x, addr, err, itype) \
@@ -620,6 +621,7 @@ do { \
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __get_kernel_nofault(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), err_label)
#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
@@ -627,6 +629,7 @@ do { \
do { \
int __kr_err; \
\
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), __kr_err); \
if (unlikely(__kr_err)) \
@@ -635,6 +638,7 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __put_kernel_nofault(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 7b32be2a3cf0..d13f1a514750 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+
+ 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,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -1971,6 +1991,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),
{}
};
--
2.34.1
On Thu, Sep 19, 2024 at 12:57 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> On Wed, Sep 18, 2024 at 8:15 PM Andrey Konovalov <andreyknvl@gmail.com> wrote:
> > You still have the same problem here.
> >
> > What I meant is:
> >
> > char *ptr;
> > char buf[128 - KASAN_GRANULE_SIZE];
> > size_t size = sizeof(buf);
> >
> > ptr = kmalloc(size, GFP_KERNEL);
> > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> >
> > KUNIT_EXPECT_KASAN_FAIL(...);
> > ...
> >
> > kfree(ptr);
>
> Thanks for catching this! I've turned kunit test into OOB instead of UAF.
> ---
> v3: changed kunit test from UAF to OOB case and git commit message.
> ---
> Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
> strncpy_from_kernel_nofault() where __put_kernel_nofault, __get_kernel_nofault
> macros are used.
>
> __get_kernel_nofault needs instrument_memcpy_before() which handles
> KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
> macro, instrument_write() check should be enough as it's validated via
> kmsan_copy_to_user() in instrument_put_user().
>
> __get_user_size was appended with instrument_get_user() for KMSAN check in
> commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
> put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
>
> copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB bug reports
> as expected for each copy_from/to_kernel_nofault call.
"as expected for each" => "as expected, one for each"
>
> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> arch/x86/include/asm/uaccess.h | 4 ++++
> mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a7755c1a441..87fb59071e8c 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -353,6 +353,7 @@ do { \
> default: \
> (x) = __get_user_bad(); \
> } \
> + instrument_get_user(x); \
> } while (0)
>
> #define __get_user_asm(x, addr, err, itype) \
> @@ -620,6 +621,7 @@ do { \
>
> #ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> #define __get_kernel_nofault(dst, src, type, err_label) \
> + instrument_memcpy_before(dst, src, sizeof(type)); \
> __get_user_size(*((type *)(dst)), (__force type __user *)(src), \
> sizeof(type), err_label)
> #else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> @@ -627,6 +629,7 @@ do { \
> do { \
> int __kr_err; \
> \
> + instrument_memcpy_before(dst, src, sizeof(type)); \
> __get_user_size(*((type *)(dst)), (__force type __user *)(src), \
> sizeof(type), __kr_err); \
> if (unlikely(__kr_err)) \
> @@ -635,6 +638,7 @@ do { \
> #endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
>
> #define __put_kernel_nofault(dst, src, type, err_label) \
> + instrument_write(dst, sizeof(type)); \
> __put_user_size(*((type *)(src)), (__force type __user *)(dst), \
> sizeof(type), err_label)
>
> diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
> index 7b32be2a3cf0..d13f1a514750 100644
> --- a/mm/kasan/kasan_test.c
> +++ b/mm/kasan/kasan_test.c
> @@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test)
> kfree(ptr);
> }
>
> +static void copy_from_to_kernel_nofault_oob(struct kunit *test)
> +{
> + char *ptr;
> + char buf[128];
> + size_t size = sizeof(buf);
> +
> + ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
> +
> + 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,
> + copy_to_kernel_nofault(ptr, &buf[0], size));
> + kfree(ptr);
> +}
> +
> static struct kunit_case kasan_kunit_test_cases[] = {
> KUNIT_CASE(kmalloc_oob_right),
> KUNIT_CASE(kmalloc_oob_left),
> @@ -1971,6 +1991,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),
> {}
> };
The test looks good to me now.
But you need to send the patch as a standalone email, without
combining it with the response to my comment.
Thanks!
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
strncpy_from_kernel_nofault() where __put_kernel_nofault,
__get_kernel_nofault macros are used.
__get_kernel_nofault needs instrument_memcpy_before() which handles
KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
macro, instrument_write() check should be enough as it's validated via
kmsan_copy_to_user() in instrument_put_user().
__get_user_size was appended with instrument_get_user() for KMSAN check in
commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB
bug reports as expected, one for each copy_from/to_kernel_nofault call.
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
v3: changed kunit test from UAF to OOB case and git commit message.
v4: updated a grammar in git commit message.
---
arch/x86/include/asm/uaccess.h | 4 ++++
mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..87fb59071e8c 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -353,6 +353,7 @@ do { \
default: \
(x) = __get_user_bad(); \
} \
+ instrument_get_user(x); \
} while (0)
#define __get_user_asm(x, addr, err, itype) \
@@ -620,6 +621,7 @@ do { \
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __get_kernel_nofault(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), err_label)
#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
@@ -627,6 +629,7 @@ do { \
do { \
int __kr_err; \
\
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), __kr_err); \
if (unlikely(__kr_err)) \
@@ -635,6 +638,7 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __put_kernel_nofault(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 7b32be2a3cf0..d13f1a514750 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1899,6 +1899,26 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+
+ 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,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -1971,6 +1991,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),
{}
};
--
2.34.1On Sat, Sep 21, 2024 at 9:09 AM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
> strncpy_from_kernel_nofault() where __put_kernel_nofault,
> __get_kernel_nofault macros are used.
>
> __get_kernel_nofault needs instrument_memcpy_before() which handles
> KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
> macro, instrument_write() check should be enough as it's validated via
> kmsan_copy_to_user() in instrument_put_user().
>
> __get_user_size was appended with instrument_get_user() for KMSAN check in
> commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
> put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
>
> copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB
> bug reports as expected, one for each copy_from/to_kernel_nofault call.
>
> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
> ---
> v3: changed kunit test from UAF to OOB case and git commit message.
> v4: updated a grammar in git commit message.
> ---
> arch/x86/include/asm/uaccess.h | 4 ++++
> mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a7755c1a441..87fb59071e8c 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -353,6 +353,7 @@ do { \
> default: \
> (x) = __get_user_bad(); \
> } \
> + instrument_get_user(x); \
> } while (0)
instrument_get_user is KMSAN-related, so I don't think this change
belongs as a part of this patch.
Perhaps Alexander can comment on whether we need to add
instrument_get_user here for KMSAN.
On Sat, Sep 21, 2024 at 9:09 AM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
>
> Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
> strncpy_from_kernel_nofault() where __put_kernel_nofault,
> __get_kernel_nofault macros are used.
>
> __get_kernel_nofault needs instrument_memcpy_before() which handles
> KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
> macro, instrument_write() check should be enough as it's validated via
> kmsan_copy_to_user() in instrument_put_user().
>
> __get_user_size was appended with instrument_get_user() for KMSAN check in
> commit 888f84a6da4d("x86: asm: instrument usercopy in get_user() and
> put_user()") but only for CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
>
> copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB
> bug reports as expected, one for each copy_from/to_kernel_nofault call.
>
> Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
> Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
I tried running the tests with this patch applied, but unfortunately
the added test fails on arm64, most likely due to missing annotations
in arm64 asm code.
We need to either mark the added test as x86-only via
KASAN_TEST_NEEDS_CONFIG_ON or add annotations for arm64.
With annotations for arm64, the test might still fail for other
architectures, but I think that's fine: hopefully relevant people will
add annotations in time. But I consider both x86 and arm64 important,
so we should keep the tests working there.
If you decide to add annotations for arm64, please also test both
KASAN_SW_TAGS and KASAN_HW_TAGS modes.
Thanks!
On Sun, Sep 22, 2024 at 1:49 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > I tried running the tests with this patch applied, but unfortunately > the added test fails on arm64, most likely due to missing annotations > in arm64 asm code. Thanks for testing it on arm64. I've checked other arch and found out that only s390, x86 are using <linux/instrumented.h> header with KASAN and friends in annotations. <linux/kasan-checks.h> is in arm64 and x86. While the current [PATCH v4] has x86 only instrumentations for __get/put_kernel_nofault, I think, we can take as an example copy_from_user solution here: https://elixir.bootlin.com/linux/v6.11-rc7/source/include/linux/uaccess.h#L162-L164 , which should be a generic instrumentation of __get/put_kernel_nofault for all arch. I can try to make a separate PATCH with this solution. > We need to either mark the added test as x86-only via > KASAN_TEST_NEEDS_CONFIG_ON or add annotations for arm64. > > With annotations for arm64, the test might still fail for other > architectures, but I think that's fine: hopefully relevant people will > add annotations in time. But I consider both x86 and arm64 important, > so we should keep the tests working there. > > If you decide to add annotations for arm64, please also test both > KASAN_SW_TAGS and KASAN_HW_TAGS modes. Please suggest if the solution above to make a generic instrumentation of __get/put_kernel_nofault is suitable. Otherwise, for this patch as you've suggested, we can add KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_X86); to make sure that kunit test is for x86 only and I can add arm64 kasan-checks with SW, HW tags in separate "mm, arm64" PATCH.
On Sun, Sep 22, 2024 at 11:26 AM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > On Sun, Sep 22, 2024 at 1:49 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > > > I tried running the tests with this patch applied, but unfortunately > > the added test fails on arm64, most likely due to missing annotations > > in arm64 asm code. > > Thanks for testing it on arm64. I've checked other arch and found out > that only s390, x86 are using <linux/instrumented.h> header with > KASAN and friends in annotations. <linux/kasan-checks.h> is in arm64 and x86. > > While the current [PATCH v4] has x86 only instrumentations for > __get/put_kernel_nofault, I think, we can take as an example copy_from_user > solution here: > > https://elixir.bootlin.com/linux/v6.11-rc7/source/include/linux/uaccess.h#L162-L164 > > , which should be a generic instrumentation of __get/put_kernel_nofault > for all arch. I can try to make a separate PATCH with this solution. _inline_copy_from_user appears to only be called for non-kernel variants of copy_from_user, so you would need something different. > > We need to either mark the added test as x86-only via > > KASAN_TEST_NEEDS_CONFIG_ON or add annotations for arm64. > > > > With annotations for arm64, the test might still fail for other > > architectures, but I think that's fine: hopefully relevant people will > > add annotations in time. But I consider both x86 and arm64 important, > > so we should keep the tests working there. > > > > If you decide to add annotations for arm64, please also test both > > KASAN_SW_TAGS and KASAN_HW_TAGS modes. > > Please suggest if the solution above to make a generic instrumentation of > __get/put_kernel_nofault is suitable. I think the approach you have taken with adding instrument_read/write into arch code is fine, we just need to do this for all arches. An alternative would be common wrapper macros that calls __get/put_kernel_nofault + instrument_read/write. > Otherwise, for this patch as you've suggested, we can add > KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_X86); > to make sure that kunit test is for x86 only and I can add arm64 kasan-checks > with SW, HW tags in separate "mm, arm64" PATCH. Sounds good too.
Instrument copy_from_kernel_nofault(), copy_to_kernel_nofault(),
strncpy_from_kernel_nofault() where __put_kernel_nofault,
__get_kernel_nofault macros are used.
__get_kernel_nofault needs instrument_memcpy_before() which handles
KASAN, KCSAN checks for src, dst address, whereas for __put_kernel_nofault
macro, instrument_write() check should be enough as it's validated via
kmsan_copy_to_user() in instrument_put_user().
copy_from_to_kernel_nofault_oob() kunit test triggers 4 KASAN OOB
bug reports as expected, one for each copy_from/to_kernel_nofault call.
Reported-by: Andrey Konovalov <andreyknvl@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210505
Signed-off-by: Sabyrzhan Tasbolatov <snovitoll@gmail.com>
---
v3: changed kunit test from UAF to OOB case and git commit message.
v4: updated a grammar in git commit message.
v5: copy_from_to_kernel_nofault_oob() works only for x86 arch,
remove instrument_get_user() from __get_user_size on
!CONFIG_CC_HAS_ASM_GOTO_OUTPUT
---
arch/x86/include/asm/uaccess.h | 3 +++
mm/kasan/kasan_test.c | 23 +++++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 3a7755c1a441..e8e5185dd65c 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -620,6 +620,7 @@ do { \
#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __get_kernel_nofault(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), err_label)
#else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
@@ -627,6 +628,7 @@ do { \
do { \
int __kr_err; \
\
+ instrument_memcpy_before(dst, src, sizeof(type)); \
__get_user_size(*((type *)(dst)), (__force type __user *)(src), \
sizeof(type), __kr_err); \
if (unlikely(__kr_err)) \
@@ -635,6 +637,7 @@ do { \
#endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
#define __put_kernel_nofault(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
__put_user_size(*((type *)(src)), (__force type __user *)(dst), \
sizeof(type), err_label)
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 567d33b493e2..c369a5b1c6a7 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1944,6 +1944,28 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_X86);
+
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+
+ 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,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -2017,6 +2039,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),
{}
};
--
2.34.1On Sun, Sep 22, 2024 at 7:57 PM Sabyrzhan Tasbolatov
<snovitoll@gmail.com> wrote:
> arch/x86/include/asm/uaccess.h | 3 +++
> mm/kasan/kasan_test.c | 23 +++++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a7755c1a441..e8e5185dd65c 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -620,6 +620,7 @@ do { \
>
> #ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> #define __get_kernel_nofault(dst, src, type, err_label) \
> + instrument_memcpy_before(dst, src, sizeof(type)); \
> __get_user_size(*((type *)(dst)), (__force type __user *)(src), \
> sizeof(type), err_label)
> #else // !CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> @@ -627,6 +628,7 @@ do { \
> do { \
> int __kr_err; \
> \
> + instrument_memcpy_before(dst, src, sizeof(type)); \
> __get_user_size(*((type *)(dst)), (__force type __user *)(src), \
> sizeof(type), __kr_err); \
> if (unlikely(__kr_err)) \
> @@ -635,6 +637,7 @@ do { \
> #endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
>
> #define __put_kernel_nofault(dst, src, type, err_label) \
> + instrument_write(dst, sizeof(type)); \
> __put_user_size(*((type *)(src)), (__force type __user *)(dst), \
> sizeof(type), err_label)
>
Instead of adding KASAN, KCSAN checks per arch macro,
here is the alternative, generic way with a wrapper.
I've tested it on x86_64 only, going to test on arm64
with KASAN_SW_TAGS, KASAN_HW_TAGS if I can do it in qemu,
and form a new patch for all arch
and this PATCH v5 for x86 only can be abandoned.
Please let me know if this wrapper is good enough,
I will see in kasan_test.c how I should use SW/HW_TAG, probably,
they should be a separate test with
KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_SW_TAGS);
---
include/linux/uaccess.h | 8 ++++++++
mm/kasan/kasan_test.c | 21 +++++++++++++++++++++
mm/maccess.c | 4 ++--
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d8e4105a2f21..1b5c23868f97 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -422,6 +422,14 @@ do { \
} while (0)
#endif
+#define __get_kernel_nofault_wrapper(dst, src, type, err_label) \
+ instrument_memcpy_before(dst, src, sizeof(type)); \
+ __get_kernel_nofault(dst, src, type, err_label); \
+
+#define __put_kernel_nofault_wrapper(dst, src, type, err_label) \
+ instrument_write(dst, sizeof(type)); \
+ __put_kernel_nofault(dst, src, type, err_label); \
+
/**
* get_kernel_nofault(): safely attempt to read from a location
* @val: read into this variable
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c
index 567d33b493e2..ae05c8858c07 100644
--- a/mm/kasan/kasan_test.c
+++ b/mm/kasan/kasan_test.c
@@ -1944,6 +1944,26 @@ static void match_all_mem_tag(struct kunit *test)
kfree(ptr);
}
+static void copy_from_to_kernel_nofault_oob(struct kunit *test)
+{
+ char *ptr;
+ char buf[128];
+ size_t size = sizeof(buf);
+
+ ptr = kmalloc(size - KASAN_GRANULE_SIZE, GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr);
+
+ 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,
+ copy_to_kernel_nofault(ptr, &buf[0], size));
+ kfree(ptr);
+}
+
static struct kunit_case kasan_kunit_test_cases[] = {
KUNIT_CASE(kmalloc_oob_right),
KUNIT_CASE(kmalloc_oob_left),
@@ -2017,6 +2037,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),
{}
};
diff --git a/mm/maccess.c b/mm/maccess.c
index 518a25667323..a3533a0d0677 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -15,7 +15,7 @@ bool __weak copy_from_kernel_nofault_allowed(const
void *unsafe_src,
#define copy_from_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __get_kernel_nofault(dst, src, type, err_label); \
+ __get_kernel_nofault_wrapper(dst, src, type, err_label);\
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
@@ -49,7 +49,7 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
#define copy_to_kernel_nofault_loop(dst, src, len, type, err_label) \
while (len >= sizeof(type)) { \
- __put_kernel_nofault(dst, src, type, err_label); \
+ __put_kernel_nofault_wrapper(dst, src, type, err_label);\
dst += sizeof(type); \
src += sizeof(type); \
len -= sizeof(type); \
--
On Mon, Sep 23, 2024 at 11:09 AM Sabyrzhan Tasbolatov <snovitoll@gmail.com> wrote: > > Instead of adding KASAN, KCSAN checks per arch macro, > here is the alternative, generic way with a wrapper. > I've tested it on x86_64 only, going to test on arm64 > with KASAN_SW_TAGS, KASAN_HW_TAGS if I can do it in qemu, > and form a new patch for all arch > and this PATCH v5 for x86 only can be abandoned. > > Please let me know if this wrapper is good enough, > I will see in kasan_test.c how I should use SW/HW_TAG, probably, > they should be a separate test with > KASAN_TEST_NEEDS_CONFIG_ON(test, CONFIG_KASAN_SW_TAGS); > --- Hello, I've sent a different patch [1] to support all arch checks, tested on x86_64 and arm64 with SW/HW_TAGS. This [PATCH v5] for the x86 only can be ignored. [1] https://lore.kernel.org/linux-mm/20240927151438.2143936-1-snovitoll@gmail.com/T/#u
© 2016 - 2024 Red Hat, Inc.