[LTP] [PATCH] Modify the test logic of mincore.

[LTP] [PATCH] Modify the test logic of mincore.

[zhanglianjie]

Currently mincore has a vulnerability and is easy to be attacked.
CVE has fixed the vulnerability.
Please see https://www.linuxkernelcves.com/cves/CVE-2019-5489

Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>
---
 testcases/kernel/syscalls/mincore/mincore04.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/testcases/kernel/syscalls/mincore/mincore04.c b/testcases/kernel/syscalls/mincore/mincore04.c
index ed0ab7dfa..345dedd9a 100644
--- a/testcases/kernel/syscalls/mincore/mincore04.c
+++ b/testcases/kernel/syscalls/mincore/mincore04.c
@@ -95,12 +95,12 @@ static void test_mincore(void)
 	locked_pages = count_pages_in_cache();
 	tst_reap_children();

-	if (locked_pages == NUM_PAGES)
-		tst_res(TPASS, "mincore reports all %d pages locked by child process "
-			"are resident", locked_pages);
-	else
-		tst_res(TFAIL, "mincore reports %d pages resident but %d pages "
+	if (locked_pages == 0)
+		tst_res(TPASS, "mincore reports %d pages resident but %d pages "
 			"locked by child process", locked_pages, NUM_PAGES);
+	else
+		tst_res(TFAIL, "mincore reports all %d pages locked by child process "
+			"are resident", locked_pages);
 }

 static struct tst_test test = {
@@ -109,4 +109,9 @@ static struct tst_test test = {
 	.forks_child = 1,
 	.test_all = test_mincore,
 	.needs_checkpoints = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "574823bfab82"},
+		{"CVE", "2019-5489"},
+		{}
+	}
 };
--
2.20.1

[LTP] [PATCH] Modify the test logic of mincore.

[Cyril Hrubis]

Hi!
> Currently mincore has a vulnerability and is easy to be attacked.
> CVE has fixed the vulnerability.
> Please see https://www.linuxkernelcves.com/cves/CVE-2019-5489
> 
> Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>
> ---
>  testcases/kernel/syscalls/mincore/mincore04.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/mincore/mincore04.c b/testcases/kernel/syscalls/mincore/mincore04.c
> index ed0ab7dfa..345dedd9a 100644
> --- a/testcases/kernel/syscalls/mincore/mincore04.c
> +++ b/testcases/kernel/syscalls/mincore/mincore04.c
> @@ -95,12 +95,12 @@ static void test_mincore(void)
>  	locked_pages = count_pages_in_cache();
>  	tst_reap_children();
> 
> -	if (locked_pages == NUM_PAGES)
> -		tst_res(TPASS, "mincore reports all %d pages locked by child process "
> -			"are resident", locked_pages);
> -	else
> -		tst_res(TFAIL, "mincore reports %d pages resident but %d pages "
> +	if (locked_pages == 0)
> +		tst_res(TPASS, "mincore reports %d pages resident but %d pages "
>  			"locked by child process", locked_pages, NUM_PAGES);
> +	else
> +		tst_res(TFAIL, "mincore reports all %d pages locked by child process "
> +			"are resident", locked_pages);
>  }

This does not make any sense, the kernel commit explicitly states that
all mapped pages are reported as in core. We do call mlock() in the
child, which will fault all the pages and lock them in memory. So the
test should work both before and after the fix as well.

The kernel commit in question weakened mincore() in a sense that it's
more likely to report pages in core than it previously was. Now all that
is needed is to fault the pages by reading some bytes from them to make
sure they are reported as in core.

If the test fails for you, something is probably broken at your end.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] Modify the test logic of mincore.

[zhanglianjie]

Hi,
>> Currently mincore has a vulnerability and is easy to be attacked.
>> CVE has fixed the vulnerability.
>> Please see https://www.linuxkernelcves.com/cves/CVE-2019-5489
>>
>> Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>
>> ---
>>   testcases/kernel/syscalls/mincore/mincore04.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/testcases/kernel/syscalls/mincore/mincore04.c b/testcases/kernel/syscalls/mincore/mincore04.c
>> index ed0ab7dfa..345dedd9a 100644
>> --- a/testcases/kernel/syscalls/mincore/mincore04.c
>> +++ b/testcases/kernel/syscalls/mincore/mincore04.c
>> @@ -95,12 +95,12 @@ static void test_mincore(void)
>>   	locked_pages = count_pages_in_cache();
>>   	tst_reap_children();
>>
>> -	if (locked_pages == NUM_PAGES)
>> -		tst_res(TPASS, "mincore reports all %d pages locked by child process "
>> -			"are resident", locked_pages);
>> -	else
>> -		tst_res(TFAIL, "mincore reports %d pages resident but %d pages "
>> +	if (locked_pages == 0)
>> +		tst_res(TPASS, "mincore reports %d pages resident but %d pages "
>>   			"locked by child process", locked_pages, NUM_PAGES);
>> +	else
>> +		tst_res(TFAIL, "mincore reports all %d pages locked by child process "
>> +			"are resident", locked_pages);
>>   }
> 
> This does not make any sense, the kernel commit explicitly states that
> all mapped pages are reported as in core. We do call mlock() in the
> child, which will fault all the pages and lock them in memory. So the
> test should work both before and after the fix as well.
> 
> The kernel commit in question weakened mincore() in a sense that it's
> more likely to report pages in core than it previously was. Now all that
> is needed is to fault the pages by reading some bytes from them to make
> sure they are reported as in core.
> 
> If the test fails for you, something is probably broken at your end.
> 

Using the CVE patch on mips, the test will fail because the child 
process mlocks all pages, but the parent process cannot obtain these 
pages through mincore and is locked.

[LTP] [PATCH] Modify the test logic of mincore.

[Cyril Hrubis]

Hi!
> > This does not make any sense, the kernel commit explicitly states that
> > all mapped pages are reported as in core. We do call mlock() in the
> > child, which will fault all the pages and lock them in memory. So the
> > test should work both before and after the fix as well.
> > 
> > The kernel commit in question weakened mincore() in a sense that it's
> > more likely to report pages in core than it previously was. Now all that
> > is needed is to fault the pages by reading some bytes from them to make
> > sure they are reported as in core.
> > 
> > If the test fails for you, something is probably broken at your end.
> > 
> 
> Using the CVE patch on mips, the test will fail because the child 
> process mlocks all pages, but the parent process cannot obtain these 
> pages through mincore and is locked.

Sounds like a kernel bug.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] Modify the test logic of mincore.

[zhanglianjie]

Hi!
>>> This does not make any sense, the kernel commit explicitly states that
>>> all mapped pages are reported as in core. We do call mlock() in the
>>> child, which will fault all the pages and lock them in memory. So the
>>> test should work both before and after the fix as well.
>>>
>>> The kernel commit in question weakened mincore() in a sense that it's
>>> more likely to report pages in core than it previously was. Now all that
>>> is needed is to fault the pages by reading some bytes from them to make
>>> sure they are reported as in core.
>>>
>>> If the test fails for you, something is probably broken at your end.
>>>
>>
>> Using the CVE patch on mips, the test will fail because the child
>> process mlocks all pages, but the parent process cannot obtain these
>> pages through mincore and is locked.
> 
> Sounds like a kernel bug.
> 
CVE list has provided repair patches, The patch I submitted this time is 
to modify the logic of the test results, do you plan to incorporate it?

[LTP] [PATCH] Modify the test logic of mincore.

[Cyril Hrubis]

Hi!
> >> Using the CVE patch on mips, the test will fail because the child
> >> process mlocks all pages, but the parent process cannot obtain these
> >> pages through mincore and is locked.
> > 
> > Sounds like a kernel bug.
> > 
> CVE list has provided repair patches, The patch I submitted this time is 
> to modify the logic of the test results, do you plan to incorporate it?

Let me try to explain it once again.

As far as I can tell the CVE kernel patch you pointed out has NO effect
on the testcase and there is NO need to modify the test at all. The test
works fine both before and after the kernel patch.

If the test fails for you YOUR kernel is broken.

That is unless you prove that the test is actually wrong, which you
haven't and I do not think that you actually can.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] Modify the test logic of mincore.

[zhanglianjie]

Hi,

> Hi!
>>>> Using the CVE patch on mips, the test will fail because the child
>>>> process mlocks all pages, but the parent process cannot obtain these
>>>> pages through mincore and is locked.
>>>
>>> Sounds like a kernel bug.
>>>
>> CVE list has provided repair patches, The patch I submitted this time is
>> to modify the logic of the test results, do you plan to incorporate it?
> 
> Let me try to explain it once again.
> 
> As far as I can tell the CVE kernel patch you pointed out has NO effect
> on the testcase and there is NO need to modify the test at all. The test
> works fine both before and after the kernel patch.
> 
> If the test fails for you YOUR kernel is broken.
> 
> That is unless you prove that the test is actually wrong, which you
> haven't and I do not think that you actually can.
> 

The test was passed before the cve patch was applied, and the test
failed after the patch was applied, and the test results were the same
on both the x86 and mips architectures.
This cve patch changes mincore() to count "mapped" pages instead of 
"cached" pages.

[LTP] [PATCH] Modify the test logic of mincore.

[Cyril Hrubis]

Hi!
> The test was passed before the cve patch was applied, and the test
> failed after the patch was applied, and the test results were the same
> on both the x86 and mips architectures.
>
> This cve patch changes mincore() to count "mapped" pages instead of 
> "cached" pages.

I've looked at at the mincore code and found what is your problem. The
initiall patch that fixed the CVE was wrong and later reverted and
replaced with a different fix:

commit 134fca9063ad4851de767d1768180e5dede9a881
Author: Jiri Kosina <jkosina@suse.cz>
Date:   Tue May 14 15:41:38 2019 -0700

    mm/mincore.c: make mincore() more conservative


So it's your kernel that is broken and the test is fine.

-- 
Cyril Hrubis
chrubis@suse.cz