Re: [PATCH v2 -next 09/18] landlock: Add READ_METADATA and WRITE_METADATA access rights

["Günther Noack" <gnoack@google.com>]

Hello!

On Fri, Oct 16, 2026 at 10:44:16AM -0400, Justin Suess wrote:
> On Thu, Oct 15, 2026 at 08:54:15PM +0800, Cai Xinchen wrote:
> > Add two new filesystem access rights to control file metadata access:
> > 
> > - LANDLOCK_ACCESS_FS_READ_METADATA: Read file or directory metadata
> >   (e.g. inode attributes, extended attributes and POSIX ACLs) through
> >   stat(2), fstat(2), lstat(2), newfstatat(2), getxattr(2) and
> >   friends, listxattr(2) and friends, and the POSIX ACL read
> >   operations.
> > - LANDLOCK_ACCESS_FS_WRITE_METADATA: Change file or directory
> >   metadata through chmod(2) and friends, chown(2) and friends,
> >   utimensat(2) and friends, setxattr(2) and friends, removexattr(2)
> >   and friends, and the POSIX ACL set and remove operations.
> > 
> > This follows the coarse-grained grouping approach discussed in the
> > mailing list [1], where file metadata operations are grouped by
> > security relevance rather than by individual syscall.
> > 
> > Implement the two rights by hooking the inode_getattr,
> > inode_file_getattr, inode_setattr, inode_file_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, POSIX ACL reads, and the
> >   chattr(1)/xfs_io style reads: ioctl(FS_IOC_GETFLAGS),
> >   ioctl(FS_IOC_FSGETXATTR) and file_getattr(2) via inode_getattr,
> >   inode_getxattr, inode_listxattr, inode_get_acl and
> >   inode_file_getattr (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,
> >   POSIX ACL set and remove, and the chattr(1)/xfs_io style writes:
> >   ioctl(FS_IOC_SETFLAGS), ioctl(FS_IOC_FSSETXATTR) and
> >   file_setattr(2) via inode_setattr, inode_file_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.
> > 
> > Stacking filesystems forward fileattr operations to underlying inodes
> > without calling the LSM hooks again (see the preceding patch), so a
> > fileattr operation on an overlayfs file is checked once on the
> > user-facing merged path and is not spuriously denied because of the
> > kernel-side creator credentials used for the forward.
> > 
> > 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.
> > 
> > Remove stat(2), chmod(2), chown(2), setxattr(2) and utime(2) from the
> > list of file-related actions that cannot be restricted yet, and add
> > the names of the new rights to _LANDLOCK_ACCESS_FS_NAMES in
> > include/linux/landlock.h, which is shared by audit records and trace
> > events, so that denied accesses can be reported with a human-readable
> > name and the static assertion on fs_access_strings in
> > security/landlock/audit.c stays valid.
> > 
> > Increment the Landlock ABI version from 12 to 13, and update the ABI
> > version check in tools/testing/selftests/landlock/base_test.c
> > accordingly.
> >
> LANDLOCK_ACCESS_FS_*_METADATA should be checked at open time and follow
> the mechanic for the truncate right. Otherwise the access for already
> opened file descriptors would be changed. See hook_file_open in
> security/landlock/fs.c and this discussion here [1]
> 
> It should have clear documentation for this as well.
> 
> Justin
> [1] https://lore.kernel.org/all/20260929.Ahgoo4yeetai@digikod.net/


I agree with Justin here.

Apart from what I said in [1], Justin and Mickaël pointed out this
other aspect in which it is similar to *_TRUNCATE, in [2] and [3]:
System calls like fstat(2) operate on an already-opened file
descriptor, and the semantic that Landlock has so far been following
for these is that the effective access rights for that file descriptor
would be determined at the time of open(2) and stored in the opened
struct file, just as it has been done for the *_TRUNCATE and
*_IOCTL_DEV access rights in the hook_file_open() call which Justin
points to above.

That way, an already opened file acts as a handle for a "capability"
(in the more general sense of the word), and the things you can do
with that opened file don't change as the process enforces nested
Landlock domains.

Landlock's documentation describes this in
https://docs.kernel.org/userspace-api/landlock.html#rights-associated-with-file-descriptors

It feels that this would be in order for the *_METADATA rights as
well?  Does this seem reasonable to you?

(If you have arguments against that, I'm also more than happy to
discuss, but we should make a conscious decision. :-))

—Günther

[1] https://lore.kernel.org/all/arurOphe6QIx2Zr8@google.com/
[2] https://lore.kernel.org/all/arvjb0HJYh1KyEBB@suesslenovo/
[3] https://lore.kernel.org/all/20260929.Ahgoo4yeetai@digikod.net/