linux-kernel - Re: [PATCH v2 1/3] mm/swapfile: make security_vm_enough_memory

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: <87letqpzm1.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Tue, 21 Jun 2022 09:35:50 +0800
From:   "Huang, Ying" <ying.huang@...el.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     <akpm@...ux-foundation.org>, <david@...hat.com>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] mm/swapfile: make security_vm_enough_memory_mm()
 work as expected

Miaohe Lin <linmiaohe@...wei.com> writes:

> On 2022/6/20 15:31, Huang, Ying wrote:
>> Miaohe Lin <linmiaohe@...wei.com> writes:
>> 
>>> security_vm_enough_memory_mm() checks whether a process has enough memory
>>> to allocate a new virtual mapping. And total_swap_pages is considered as
>>> available memory while swapoff tries to make sure there's enough memory
>>> that can hold the swapped out memory. But total_swap_pages contains the
>>> swap space that is being swapoff. So security_vm_enough_memory_mm() will
>>> success even if there's no memory to hold the swapped out memory because
>>> total_swap_pages always greater than or equal to p->pages.
>> 
>> Per my understanding, swapoff will not allocate virtual mapping by
>> itself.  But after swapoff, the overcommit limit could be exceeded.
>> security_vm_enough_memory_mm() is used to check that.  For example, in a
>> system with 4GB memory and 8GB swap, and 10GB is in use,
>> 
>> CommitLimit:    4+8 = 12GB
>> Committed_AS:   10GB
>> 
>> security_vm_enough_memory_mm() in swapoff() will fail because
>> 10+8 = 18 > 12.  This is expected because after swapoff, the overcommit
>> limit will be exceeded.
>> 
>> If 3GB is in use,
>> 
>> CommitLimit:    4+8 = 12GB
>> Committed_AS:   3GB
>> 
>> security_vm_enough_memory_mm() in swapoff() will succeed because
>> 3+8 = 11 < 12.  This is expected because after swapoff, the overcommit
>> limit will not be exceeded.
>
> In OVERCOMMIT_NEVER scene, I think you're right.
>
>> 
>> So, what's the real problem of the original implementation?  Can you
>> show it with an example as above?
>
> In OVERCOMMIT_GUESS scene, in a system with 4GB memory and 8GB swap, and 10GB is in use,
> pages below is 8GB, totalram_pages() + total_swap_pages is 12GB, so swapoff() will succeed
> instead of expected failure because 8 < 12. The overcommit limit is always *ignored* in the
> below case.
>
> 	if (sysctl_overcommit_memory == OVERCOMMIT_GUESS) {
> 		if (pages > totalram_pages() + total_swap_pages)
> 			goto error;
> 		return 0;
> 	}
>
> Or am I miss something?

Per my understanding, with OVERCOMMIT_GUESS, the number of in-use pages
isn't checked at all.  The only restriction is that the size of the
virtual mapping created should be less than total RAM + total swap
pages.  Because swapoff() will not create virtual mapping, so it's
expected that security_vm_enough_memory_mm() in swapoff() always
succeeds.

Best Regards,
Huang, Ying

>
> Thanks!
>
>> 
>>> In order to fix it, p->pages should be retracted from total_swap_pages
>>> first and then check whether there's enough memory for inuse swap pages.
>>>
>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>> 
>> [snip]
>> 
>> .
>>