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

["David Hildenbrand (Arm)" <david@kernel.org>]

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".

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.


> 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?


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

-- 
Cheers,

David