Re: [PATCH 05/16] hugetlbfs: enforce F_SEAL_EXEC in setattr

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

On 10/27/26 03:22, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] hugetlbfs: uninitialized info->seals read in hugetlbfs_setattr() on root dir
> --
> 
> commit dfab7a0d71278312f805e82ce52518a8ea2f538d
> Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
> 
> hugetlbfs: enforce F_SEAL_EXEC in setattr
> 
> This patch updates hugetlbfs_setattr() to call memfd_check_seals_mode() when
> ATTR_MODE is set so that fchmod() cannot modify execute bits on a hugetlbfs
> memfd sealed with F_SEAL_EXEC.
> 
> Link: https://patch.msgid.link/20260629180247.2343229-1-eva.kurchatova@virtuozzo.com
> 
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index 6656807ce4377..7f5c3962b5d56 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
> 
> [ ... ]
> 
>> @@ -841,6 +842,13 @@ static int hugetlbfs_setattr(const struct mnt_idmap *idmap,
>>  	if (error)
>>  		return error;
>>  
>> +	if (ia_valid & ATTR_MODE) {
>> +		error = memfd_check_seals_mode(info->seals, inode->i_mode,
>> +					       attr->ia_mode);
>> +		if (error)
>> +			return error;
>> +	}
> 
> [Severity: Medium]
> Can this read uninitialized or stale slab memory from info->seals when
> chmod() is called on a hugetlbfs root directory?
> 
> hugetlbfs_dir_inode_operations wires .setattr to hugetlbfs_setattr(). Unlike
> hugetlbfs_get_inode(), which sets info->seals = F_SEAL_SEAL,
> hugetlbfs_get_root() allocates the root directory inode via new_inode() ->
> hugetlbfs_alloc_inode() without initializing HUGETLBFS_I(inode)->seals:
> 
> fs/hugetlbfs/inode.c:hugetlbfs_get_root() {
> 	inode = new_inode(sb);
> 	if (inode) {
> 		...
> 		inode->i_op = &hugetlbfs_dir_inode_operations;
> 		inode->i_fop = &simple_dir_operations;
> 		HUGETLBFS_I(inode)->resv_map = NULL;
> 		/* directory inodes start off with i_nlink == 2 (for "." entry) */
> 		inc_nlink(inode);
> 		lockdep_annotate_inode_mutex_key(inode);
> 	}
> 	return inode;
> }
> 
> Because hugetlbfs_inode_cachep uses the init_once() slab constructor, which
> only calls inode_init_once(&ei->vfs_inode), newly allocated or recycled
> hugetlbfs_inode_info objects are not zeroed on allocation.
> 
> Previously, hugetlbfs_setattr() only checked info->seals under ATTR_SIZE,
> which is not reachable on directories. With the new ATTR_MODE check, changing
> any execute/search bit on the mountpoint root directory reads info->seals
> and can fail with -EPERM if the F_SEAL_EXEC bit is set in uninitialized
> memory or left over from a previously freed sealed hugetlb memfd.
> 
> Should hugetlbfs_get_root() also initialize HUGETLBFS_I(inode)->seals =
> F_SEAL_SEAL?

Shouldn't we just skip the check for directories?


diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 7f5c3962b5d56..293c7965f4894 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -842,7 +842,7 @@ static int hugetlbfs_setattr(const struct mnt_idmap *idmap,
        if (error)
                return error;

-       if (ia_valid & ATTR_MODE) {
+       if (S_ISREG(inode->i_mode) && (ia_valid & ATTR_MODE)) {
                error = memfd_check_seals_mode(info->seals, inode->i_mode,
                                               attr->ia_mode);
                if (error)

Because sealing of directories does not apply.

Thoughts?

-- 
Cheers,

David