Re: [PATCH RFC -next 09/12] landlock: Implement metadata access hooks

["Günther Noack" <gnoack3000@gmail.com>]

On Thu, Sep 24, 2026 at 06:48:28PM +0800, Cai Xinchen wrote:
> Implement the LANDLOCK_ACCESS_FS_READ_METADATA and
> LANDLOCK_ACCESS_FS_WRITE_METADATA access rights by hooking the
> inode_getattr, inode_setattr, inode_setxattr, inode_getxattr,
> inode_listxattr, inode_removexattr, inode_set_acl, inode_get_acl and
> inode_remove_acl LSM hooks, which now receive a struct path thanks to
> the preceding VFS and LSM refactoring.
> 
> The following system calls are now controlled:
> 
> - stat(2), fstat(2), lstat(2), newfstatat(2), getxattr(2) and
>   friends, listxattr(2) and friends, and POSIX ACL reads via
>   inode_getattr, inode_getxattr, inode_listxattr and inode_get_acl
>   (READ_METADATA)
> - chmod(2), fchmod(2), fchmodat(2), fchmodat2(2), chown(2), fchown(2),
>   lchown(2), fchownat(2), chgrp(2), utimensat(2), futimens(2),
>   utime(2), setxattr(2) and friends, removexattr(2) and friends, and
>   POSIX ACL set and remove via inode_setattr, inode_setxattr,
>   inode_removexattr, inode_set_acl and inode_remove_acl
>   (WRITE_METADATA)
> 
> Both new rights are added to ACCESS_FILE as they apply to both files
> and directories.
> 
> hook_inode_setattr only restricts explicit metadata changes, i.e. it
> checks WRITE_METADATA only when the ia_valid mask contains
> ATTR_MODE, ATTR_UID, ATTR_GID, ATTR_TIMES_SET or ATTR_TOUCH.
> Metadata changes that the kernel performs implicitly, such as
> timestamp updates on write(2) or size changes on truncate(2), are
> therefore not restricted, and neither are chmod(2)/chown(2) calls
> that do not change any attribute (e.g. chown(2) with -1/-1, which is
> a no-op that never reaches the hook), matching the behavior of the
> SELinux inode_setattr hook.
> 
> Kernel-internal accesses performed with override_creds() (e.g.
> overlayfs and cachefiles) are not affected because Landlock domains
> are attached to credentials, and kernel threads without a Landlock
> domain (e.g. nfsd and ksmbd) are not restricted either.
> 
> Assisted-by: opencode: glm-5.3
> Signed-off-by: Cai Xinchen <caixinchen1@huawei.com>
> ---
>  security/landlock/fs.c | 86 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index cab43892ec2f..e58b2aa0da65 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -318,6 +318,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_EXECUTE | \
>  	LANDLOCK_ACCESS_FS_WRITE_FILE | \
>  	LANDLOCK_ACCESS_FS_READ_FILE | \
> +	LANDLOCK_ACCESS_FS_READ_METADATA | \
> +	LANDLOCK_ACCESS_FS_WRITE_METADATA | \
>  	LANDLOCK_ACCESS_FS_TRUNCATE | \
>  	LANDLOCK_ACCESS_FS_IOCTL_DEV | \
>  	LANDLOCK_ACCESS_FS_RESOLVE_UNIX)
> @@ -1676,6 +1678,81 @@ static int hook_path_truncate(const struct path *const path)
>  	return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>  }
>  
> +static int hook_inode_getattr(const struct path *const path)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_READ_METADATA);
> +}
> +
> +static int hook_inode_setattr(const struct path *const path,
> +			      struct iattr *const attr)
> +{
> +	/*
> +	 * Explicit metadata changes (i.e. mode, ownership, and timestamps
> +	 * set with utimes() and friends) require
> +	 * LANDLOCK_ACCESS_FS_WRITE_METADATA.  Implicit timestamp updates
> +	 * (e.g. ATTR_CTIME set for a write) and size changes (handled by
> +	 * the truncate hooks) are not restricted.

Nit: It feels like this comment about implicit timestamp updates
(especially the size change) should go in the top-level documentation
for the WRITE_METADATA right?  setattr() can not result in a size
change, after all, AFAIK?

Remark on the side, apart from truncation, normal writes into the file
can of course also change its size ;-) and ATTR_ATIME and ATTR_MTIME
also come to mind as implicit metadata changes.

> +	 */
> +	if (!(attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
> +				ATTR_TIMES_SET | ATTR_TOUCH)))
> +		return 0;
> +
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}
> +
> +static int hook_inode_setxattr(const struct path *const path,
> +			       const char *const name,
> +			       const void *const value, const size_t size,
> +			       const int flags)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}
> +
> +static int hook_inode_getxattr(const struct path *const path,
> +			       const char *const name)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_READ_METADATA);
> +}
> +
> +static int hook_inode_listxattr(const struct path *const path)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_READ_METADATA);
> +}
> +
> +static int hook_inode_removexattr(const struct path *const path,
> +				  const char *const name)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}
> +
> +static int hook_inode_set_acl(const struct path *const path,
> +			      const char *const acl_name,
> +			      struct posix_acl *const kacl)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}
> +
> +static int hook_inode_get_acl(const struct path *const path,
> +			      const char *const acl_name)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_READ_METADATA);
> +}
> +
> +static int hook_inode_remove_acl(const struct path *const path,
> +				 const char *const acl_name)
> +{
> +	return current_check_access_path(path,
> +					LANDLOCK_ACCESS_FS_WRITE_METADATA);
> +}
> +
>  /**
>   * unmask_scoped_access - Remove access right bits in @masks in all layers
>   *                        where @client and @server have the same domain
> @@ -2100,6 +2177,15 @@ static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>  	LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>  	LSM_HOOK_INIT(path_truncate, hook_path_truncate),
> +	LSM_HOOK_INIT(inode_getattr, hook_inode_getattr),
> +	LSM_HOOK_INIT(inode_setattr, hook_inode_setattr),
> +	LSM_HOOK_INIT(inode_setxattr, hook_inode_setxattr),
> +	LSM_HOOK_INIT(inode_getxattr, hook_inode_getxattr),
> +	LSM_HOOK_INIT(inode_listxattr, hook_inode_listxattr),
> +	LSM_HOOK_INIT(inode_removexattr, hook_inode_removexattr),
> +	LSM_HOOK_INIT(inode_set_acl, hook_inode_set_acl),
> +	LSM_HOOK_INIT(inode_get_acl, hook_inode_get_acl),
> +	LSM_HOOK_INIT(inode_remove_acl, hook_inode_remove_acl),
>  	LSM_HOOK_INIT(unix_find, hook_unix_find),
>  
>  	LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security),
> -- 
> 2.18.0.huawei.25
> 

On the Landlock side, the implementation looks quite straightforward,
without having double checked for missing hooks now.  This commit is
probably fine as soon as we have agreement on the LSM hook changes.

–Günther