Message-ID: <8b4e31cf-de20-703c-4b53-ad86d4282a37@redhat.com>
Date:   Mon, 23 Jan 2023 13:53:46 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Content-Language: en-US
To:     Catalin Marinas <catalin.marinas@arm.com>
Cc:     Joey Gouly <joey.gouly@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Lennart Poettering <lennart@poettering.net>,
        =?UTF-8?Q?Zbigniew_J=c4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Kees Cook <keescook@chromium.org>,
        Szabolcs Nagy <szabolcs.nagy@arm.com>,
        Mark Brown <broonie@kernel.org>,
        Jeremy Linton <jeremy.linton@arm.com>,
        Topi Miettinen <toiwoton@gmail.com>, linux-mm@kvack.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-abi-devel@lists.sourceforge.net, nd@arm.com, shuah@kernel.org
References: <20230119160344.54358-1-joey.gouly@arm.com>
 <20230119160344.54358-2-joey.gouly@arm.com>
 <4a1faf67-178e-c9ba-0db1-cf90408b0d7d@redhat.com> <Y857Uoq7fjO0lZ12@arm.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v2 1/2] mm: Implement memory-deny-write-execute as a prctl
In-Reply-To: <Y857Uoq7fjO0lZ12@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 23.01.23 13:19, Catalin Marinas wrote:
> On Mon, Jan 23, 2023 at 12:45:50PM +0100, David Hildenbrand wrote:
>> On 19.01.23 17:03, Joey Gouly wrote:
>>> The aim of such policy is to prevent a user task from creating an
>>> executable mapping that is also writeable.
>>>
>>> An example of mmap() returning -EACCESS if the policy is enabled:
>>>
>>> 	mmap(0, size, PROT_READ | PROT_WRITE | PROT_EXEC, flags, 0, 0);
>>>
>>> Similarly, mprotect() would return -EACCESS below:
>>>
>>> 	addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0);
>>> 	mprotect(addr, size, PROT_READ | PROT_WRITE | PROT_EXEC);
>>>
>>> The BPF filter that systemd MDWE uses is stateless, and disallows
>>> mprotect() with PROT_EXEC completely. This new prctl allows PROT_EXEC to
>>> be enabled if it was already PROT_EXEC, which allows the following case:
>>>
>>> 	addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0);
>>> 	mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI);
>>>
>>> where PROT_BTI enables branch tracking identification on arm64.
>>>
>>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>>> Co-developed-by: Catalin Marinas <catalin.marinas@arm.com>
>>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>>    include/linux/mman.h           | 34 ++++++++++++++++++++++++++++++++++
>>>    include/linux/sched/coredump.h |  6 +++++-
>>>    include/uapi/linux/prctl.h     |  6 ++++++
>>>    kernel/sys.c                   | 33 +++++++++++++++++++++++++++++++++
>>>    mm/mmap.c                      | 10 ++++++++++
>>>    mm/mprotect.c                  |  5 +++++
>>>    6 files changed, 93 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/mman.h b/include/linux/mman.h
>>> index 58b3abd457a3..cee1e4b566d8 100644
>>> --- a/include/linux/mman.h
>>> +++ b/include/linux/mman.h
>>> @@ -156,4 +156,38 @@ calc_vm_flag_bits(unsigned long flags)
>>>    }
>>>    unsigned long vm_commit_limit(void);
>>> +
>>> +/*
>>> + * Denies creating a writable executable mapping or gaining executable permissions.
>>> + *
>>> + * This denies the following:
>>> + *
>>> + * 	a)	mmap(PROT_WRITE | PROT_EXEC)
>>> + *
>>> + *	b)	mmap(PROT_WRITE)
>>> + *		mprotect(PROT_EXEC)
>>> + *
>>> + *	c)	mmap(PROT_WRITE)
>>> + *		mprotect(PROT_READ)
>>> + *		mprotect(PROT_EXEC)
>>> + *
>>> + * But allows the following:
>>> + *
>>> + *	d)	mmap(PROT_READ | PROT_EXEC)
>>> + *		mmap(PROT_READ | PROT_EXEC | PROT_BTI)
>>> + */
>>
>> Shouldn't we clear VM_MAYEXEC at mmap() time such that we cannot set VM_EXEC
>> anymore? In an ideal world, there would be no further mprotect changes
>> required.
> 
> I don't think it works for this scenario. We don't want to disable
> PROT_EXEC entirely, only disallow it if the mapping is not already
> executable. The below should be allowed:
> 
> 	addr = mmap(0, size, PROT_READ | PROT_EXEC, flags, 0, 0);
> 	mprotect(addr, size, PROT_READ | PROT_EXEC | PROT_BTI);
> 
> but IIUC what you meant, it fails if we cleared VM_MAYEXEC at mmap()
> time.

Yeah, if you allow write access at mmap time, clear VM_MAYEXEC (and 
disallow VM_EXEC of course). But I guess we'd have to go one step 
further: if we allow exec access at mmap time, clear VM_MAYWRITE (and 
disallow VM_WRITE of course).

That at least would be then similar to how we handle mmaped files: if 
the file is not executable, we clear VM_MAYEXEC. If the file is not 
writable, we clear VM_MAYWRITE.


Clearing VM_MAYWRITE would imply that also writes via /proc/self/mem to 
such memory would be forbidden, which might also be what we are trying 
to achieve, or is that expected to still work? But clearing VM_MAYWRITE 
would mean that is_cow_mapping() would no longer fire for some VMAs, and 
we'd have to check if that's fine in all cases.


Having that said, this patch handles the case when the prctl is applied 
to a process after already having created some writable or executable 
mappings, to at least forbid if afterwards on these mappings. What is 
expected to happen if the process already has writable mappings that are 
executable at the time we enable the prctl?

Clarifying what the expected semantics with /proc/self/mem are would be 
nice.

> 
> We could clear VM_MAYEXEC if the mapping was made VM_WRITE (either by
> mmap() or mprotect()) but IIRC we concluded that this should be an
> additional prctl() flag.

Yes, I agree with enabling this restriction on a per-process level. My 
remarks only apply to processes with this toggle enabled.

-- 
Thanks,

David / dhildenb