Re: [PATCH v3 04/15] mm/shmem: honour F_SEAL_EXEC when an ACL is set

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

On 10/28/26 11:06, Balbir Singh wrote:
> On Wed, Oct 28, 2026 at 10:55:02AM +0100, David Hildenbrand (Arm) wrote:
>> From: Chris Mason <mason@kernel.org>
>>
>> The owner of a memfd sealed with F_SEAL_EXEC can still change its
>> execute bits by setting a POSIX access ACL. With CONFIG_TMPFS_POSIX_ACL,
>> this lets an unprivileged owner make a non-executable memfd executable,
>> bypassing vm.memfd_noexec=2.
>>
>> simple_set_acl() updates i_mode directly, bypassing the seal check in
>> shmem_setattr(). Add shmem_set_acl() to check the resulting mode. Share
>> the F_SEAL_EXEC check with shmem_setattr() using memfd_check_seals_mode()
>> in include/linux/memfd.h.
>>
>> Fix generated by an LLM and sent by Chris Mason in private.
>>
>> Fixes: 6fd7353829ca ("mm/memfd: add F_SEAL_EXEC")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Chris Mason <mason@kernel.org>
>> Acked-by: Pedro Falcato <pfalcato@suse.de>
>> Acked-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
>> Acked-by: Zi Yan <ziy@nvidia.com>
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Reviewed-by: Pratyush Yadav (Google) <pratyush@kernel.org>
>> Tested-by: Gregory Price (Meta) <gourry@gourry.net>
>> [ david: use common helper; simplify description ]
>> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
>> ---
>>  include/linux/memfd.h |  9 +++++++++
>>  mm/shmem.c            | 35 ++++++++++++++++++++++++++++++-----
>>  2 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/memfd.h b/include/linux/memfd.h
>> index c159e40e3f34..272479317cf1 100644
>> --- a/include/linux/memfd.h
>> +++ b/include/linux/memfd.h
>> @@ -2,10 +2,19 @@
>>  #ifndef __LINUX_MEMFD_H
>>  #define __LINUX_MEMFD_H
>>  
>> +#include <linux/fcntl.h>
>>  #include <linux/file.h>
>>  
>>  #define MEMFD_ANON_NAME "[memfd]"
>>  
>> +static inline int memfd_check_seals_mode(unsigned int seals, umode_t old_mode,
>> +		umode_t new_mode)
>> +{
>> +	if ((seals & F_SEAL_EXEC) && ((old_mode ^ new_mode) & 0111))
>> +		return -EPERM;
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_MEMFD_CREATE
>>  extern long memfd_fcntl(struct file *file, unsigned int cmd, unsigned int arg);
>>  struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx);
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index af21a89abb0f..e3e4bbbe1d80 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -77,6 +77,7 @@ static struct vfsmount *shm_mnt __ro_after_init;
>>  #include <linux/magic.h>
>>  #include <linux/syscalls.h>
>>  #include <linux/fcntl.h>
>> +#include <linux/memfd.h>
>>  #include <uapi/linux/memfd.h>
>>  #include <linux/rmap.h>
>>  #include <linux/uuid.h>
>> @@ -1546,10 +1547,11 @@ static int shmem_setattr(const struct mnt_idmap *idmap,
>>  	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 (attr->ia_valid & ATTR_MODE) {
>> +		error = memfd_check_seals_mode(info->seals, inode->i_mode,
>> +					       attr->ia_mode);
>> +		if (error)
>> +			return error;
>>  	}
>>  
>>  	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
>> @@ -5404,12 +5406,35 @@ static const struct file_operations shmem_file_operations = {
>>  #endif
>>  };
>>  
>> +#ifdef CONFIG_TMPFS_POSIX_ACL
>> +static int shmem_set_acl(const struct mnt_idmap *idmap, struct dentry *dentry,
>> +		struct posix_acl *acl, int type)
>> +{
>> +	struct inode *inode = d_inode(dentry);
>> +	umode_t mode = inode->i_mode;
>> +	int error;
>> +
>> +	if (type == ACL_TYPE_ACCESS) {
>> +		error = posix_acl_equiv_mode(acl, &mode);
>> +		if (error < 0)
>> +			return error;
>> +		error = memfd_check_seals_mode(SHMEM_I(inode)->seals,
>> +					       inode->i_mode, mode);
>> +		if (error)
>> +			return error;
>> +	}
>> +	return simple_set_acl(idmap, dentry, acl, type);
>> +}
>> +#else
>> +#define shmem_set_acl NULL
>> +#endif
>> +
>>  static const struct inode_operations shmem_inode_operations = {
>>  	.getattr	= shmem_getattr,
>>  	.setattr	= shmem_setattr,
>>  #ifdef CONFIG_TMPFS_XATTR
>>  	.listxattr	= shmem_listxattr,
>> -	.set_acl	= simple_set_acl,
>> +	.set_acl	= shmem_set_acl,
> 
> If CONFIG_TMPFS_POSIX_ACL is not defined, do we want to set the set_acl
> callback to NULL? Earlier it was simple_set_acl

Excellent question: because simple_set_acl is NULL for !CONFIG_FS_POSIX_ACL.
It's clearer to jsut keep using shmem_set_acl here, thanks!

-- 
Cheers,

David