On Mon, Feb 24, 2025 at 11:00 AM Andrey Albershteyn <aalbersh@xxxxxxxxxx> wrote:
On 2025-02-21 16:08:33, Mickaël Salaün wrote:
It looks security checks are missing. With IOCTL commands, file
permissions are checked at open time, but with these syscalls the path
is only resolved but no specific access seems to be checked (except
inode_owner_or_capable via vfs_fileattr_set).
...
On Tue, Feb 11, 2025 at 06:22:47PM +0100, Andrey Albershteyn wrote:
...
+SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
+ struct fsxattr __user *, fsx, unsigned int, at_flags)
+{
+ CLASS(fd, dir)(dfd);
+ struct fileattr fa;
+ struct path filepath;
+ int error;
+ unsigned int lookup_flags = 0;
+
+ if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ if (at_flags & AT_SYMLINK_FOLLOW)
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ if (at_flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
+
+ if (fd_empty(dir))
+ return -EBADF;
+
+ if (copy_fsxattr_from_user(&fa, fsx))
+ return -EFAULT;
+
+ error = user_path_at(dfd, filename, lookup_flags, &filepath);
+ if (error)
+ return error;
+
+ error = mnt_want_write(filepath.mnt);
+ if (!error) {
security_inode_setattr() should probably be called too.
Aren't those checks for something different - inode attributes
ATTR_*?
(sorry, the naming can't be more confusing)
Looking into security_inode_setattr() it seems to expect struct
iattr, which works with inode attributes (mode, time, uid/gid...).
These new syscalls work with filesystem inode extended flags/attributes
FS_XFLAG_* in fsxattr->fsx_xflags. Let me know if I missing
something here
A valid point. While these are two different operations, with
different structs/types, I suspect that most LSMs will consider them
to be roughly equivalent from an access control perspective, which is
why I felt the existing security_inode_{set,get}attr() hooks seemed
appropriate. However, there likely is value in keeping the ATTR and
FSX operations separate; those LSMs that wish to treat them the same
can easily do so in their respective LSM callbacks.
With all this in mind, I think it probably makes sense to create two
new LSM hooks, security_inode_{get,set}fsxattr(). The get hook should
probably be placed inside vfs_fileattr_get() just before the call to
the inode's fileattr_get() method, and the set hook should probably be
placed inside vfs_fileattr_set(), inside the inode lock and after a
successful call to fileattr_set_prepare().
Does that sound better to everyone?
--
paul-moore.com
