Re: [PATCH] memfd: deny writeable mappings when implying SEAL_WRITE

[Pratyush Yadav <pratyush@kernel.org>]

On Fri, May 08 2026, David Hildenbrand (Arm) wrote:

> On 5/5/26 15:39, Pratyush Yadav wrote:
>> From: "Pratyush Yadav (Google)" <pratyush@kernel.org>
>> 
>> When SEAL_EXEC is added, SEAL_WRITE is implied to make W^X. 
>
> I don't quite understand that.
>
> I guess what you mean is "SEAL_EXEC implies SEAL_WRITE if the file is
> executable, to prevent W^X after sealing".

Yes, exactly. If the file is executable and SEAL_EXEC is set, SEAL_WRITE
is also set to make sure the executable code is not writeable.

>
> Because if the file is not executable, there is no sealing of writes happening?
>
> It's rather odd to combine both things, though. Likely the callers should just
> have requested SEAL_WRITE.
>
> But I guess we are stuck with this mess.

Yep :-/

>
>
>> But the
>> implied seal is set after the check that makes sure the memfd can not
>> have any writable mappings. This means one can use SEAL_EXEC to apply
>> SEAL_WRITE while having writeable mappings.
>> 
>> This breaks the contract that SEAL_WRITE provides and can be used by an
>> attacker to pass a memfd that appears to be write sealed but can still
>> be modified arbitrarily.
>> 
>> Fix this by adding the implied seals before the call for
>> mapping_deny_writable() is done.
>> 
>> Fixes: c4f75bc8bd6b ("mm/memfd: add write seals when apply SEAL_EXEC to executable memfd")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Pratyush Yadav (Google) <pratyush@kernel.org>
>> ---
>>  mm/memfd.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index fb425f4e315f..abe13b291ddc 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -283,6 +283,12 @@ int memfd_add_seals(struct file *file, unsigned int seals)
>>  		goto unlock;
>>  	}
>>  
>> +	/*
>> +	 * SEAL_EXEC implies SEAL_WRITE, making W^X from the start.
>> +	 */
>> +	if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
>> +		seals |= F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE;
>> +
>>  	if ((seals & F_SEAL_WRITE) && !(*file_seals & F_SEAL_WRITE)) {
>>  		error = mapping_deny_writable(file->f_mapping);
>>  		if (error)
>> @@ -295,12 +301,6 @@ int memfd_add_seals(struct file *file, unsigned int seals)
>>  		}
>>  	}
>>  
>> -	/*
>> -	 * SEAL_EXEC implies SEAL_WRITE, making W^X from the start.
>> -	 */
>> -	if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
>> -		seals |= F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE;
>> -
>>  	*file_seals |= seals;
>>  	error = 0;
>>  
>
> Given the weird semantics, this makes sense to me.
>
> Do we have to update documentation to reflect this? But staring at the man page
> [1] we don't even seem to document F_SEAL_EXEC?

I discovered the same when trying to read more about F_SEAL_EXEC. I've
never written man pages but I suppose I can give this a shot.

>
>
> [1] https://www.man7.org/linux/man-pages/man2/F_ADD_SEALS.2const.html

-- 
Regards,
Pratyush Yadav