linux-hardening - Re: [PATCH v3] mm/memfd: add F_SEAL

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: <202212021443.0F684E33@keescook>
Date:   Fri, 2 Dec 2022 14:43:54 -0800
From:   Kees Cook <keescook@...omium.org>
To:     jeffxu@...omium.org
Cc:     skhan@...uxfoundation.org, akpm@...ux-foundation.org,
        dmitry.torokhov@...il.com, dverkamp@...omium.org, hughd@...gle.com,
        jeffxu@...gle.com, jorgelo@...omium.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-mm@...ck.org, mnissler@...omium.org, jannh@...gle.com,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3] mm/memfd: add F_SEAL_EXEC

On Fri, Dec 02, 2022 at 01:33:59AM +0000, jeffxu@...omium.org wrote:
> From: Daniel Verkamp <dverkamp@...omium.org>
> 
> The new F_SEAL_EXEC flag will prevent modification of the exec bits:
> written as traditional octal mask, 0111, or as named flags, S_IXUSR |
> S_IXGRP | S_IXOTH. Any chmod(2) or similar call that attempts to modify
> any of these bits after the seal is applied will fail with errno EPERM.
> 
> This will preserve the execute bits as they are at the time of sealing,
> so the memfd will become either permanently executable or permanently
> un-executable.
> 
> Co-developed-by: Jeff Xu <jeffxu@...omium.org>
> Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> Signed-off-by: Daniel Verkamp <dverkamp@...omium.org>
> ---
>  include/uapi/linux/fcntl.h | 1 +
>  mm/memfd.c                 | 2 ++
>  mm/shmem.c                 | 6 ++++++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 2f86b2ad6d7e..e8c07da58c9f 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -43,6 +43,7 @@
>  #define F_SEAL_GROW	0x0004	/* prevent file from growing */
>  #define F_SEAL_WRITE	0x0008	/* prevent writes */
>  #define F_SEAL_FUTURE_WRITE	0x0010  /* prevent future writes while mapped */
> +#define F_SEAL_EXEC	0x0020  /* prevent chmod modifying exec bits */
>  /* (1U << 31) is reserved for signed error codes */
>  
>  /*
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 08f5f8304746..4ebeab94aa74 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -147,6 +147,7 @@ static unsigned int *memfd_file_seals_ptr(struct file *file)
>  }
>  
>  #define F_ALL_SEALS (F_SEAL_SEAL | \
> +		     F_SEAL_EXEC | \
>  		     F_SEAL_SHRINK | \
>  		     F_SEAL_GROW | \
>  		     F_SEAL_WRITE | \
> @@ -175,6 +176,7 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>  	 *   SEAL_SHRINK: Prevent the file from shrinking
>  	 *   SEAL_GROW: Prevent the file from growing
>  	 *   SEAL_WRITE: Prevent write access to the file
> +	 *   SEAL_EXEC: Prevent modification of the exec bits in the file mode
>  	 *
>  	 * As we don't require any trust relationship between two parties, we
>  	 * must prevent seals from being removed. Therefore, sealing a file
> diff --git a/mm/shmem.c b/mm/shmem.c
> index c1d8b8a1aa3b..e18a9cf9d937 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1085,6 +1085,12 @@ static int shmem_setattr(struct user_namespace *mnt_userns,
>  	if (error)
>  		return error;
>  
> +	if ((info->seals & F_SEAL_EXEC) && (attr->ia_valid & ATTR_MODE)) {
> +		if ((inode->i_mode ^ attr->ia_mode) & 0111) {
> +			return -EPERM;
> +		}
> +	}
> +
>  	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
>  		loff_t oldsize = inode->i_size;
>  		loff_t newsize = attr->ia_size;
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

This looks sensible to me!

Reviewed-by: Kees Cook <keescook@...omium.org>

-- 
Kees Cook