| Message ID | 170900012232.938660.16382530364290848736.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] xfs: hide private inodes from bulkstat and handle functions | expand |
On Mon, Feb 26, 2024 at 06:24:46PM -0800, Darrick J. Wong wrote: > Callers are not allowed to link these > files into the directory tree, which should suffice to make these > private inodes actually private. I'm a bit confused about this commit log. The only files with i_nlink == 0 that can be linked into the namespace or O_TMPFILE files that have I_LINKABLE. The only think that cares about S_PRIVATE are the security modules (and reiserfs for it's own xattr inodes). So I think setting the flag is a good thing and gets us out of nasty interaction with LSMs, but the commit log could use a little update.
On Tue, Feb 27, 2024 at 10:15:32AM -0800, Christoph Hellwig wrote: > On Mon, Feb 26, 2024 at 06:24:46PM -0800, Darrick J. Wong wrote: > > Callers are not allowed to link these > > files into the directory tree, which should suffice to make these > > private inodes actually private. > > I'm a bit confused about this commit log. The only files with > i_nlink == 0 that can be linked into the namespace or O_TMPFILE > files that have I_LINKABLE. > > The only think that cares about S_PRIVATE are the security modules > (and reiserfs for it's own xattr inodes). > > So I think setting the flag is a good thing and gets us out of nasty > interaction with LSMs, but the commit log could use a little update. How about: "We're about to start adding functionality that uses internal inodes that are private to XFS. What this means is that userspace should never be able to access any information about these files, and should not be able to open these files by handle. "Callers must not be allowed to link these files into the directory tree, which should suffice to keep these private inodes actually private. I_LINKABLE is therefore left unset. "To prevent mis-interactions with LSMs, and the rest of the security apparatus, set S_PRIVATE." --D
On Wed, Feb 28, 2024 at 08:52:27AM -0800, Darrick J. Wong wrote: > that are private to XFS. What this means is that userspace should never > be able to access any information about these files, and should not be > able to open these files by handle. > > "Callers must not be allowed to link these files into the directory > tree, which should suffice to keep these private inodes actually > private. I_LINKABLE is therefore left unset. I_LINKABLE is only set for O_TMPFILE, so I wouldn't even bother with that. But thinking about this: what i_nlink do these private inodes have? If it is >= 1, we probably want to add an IS_PRIVATE check to xfs_link just in case they ever leak out to a place where ->link could be called.
On Wed, Feb 28, 2024 at 09:02:30AM -0800, Christoph Hellwig wrote: > On Wed, Feb 28, 2024 at 08:52:27AM -0800, Darrick J. Wong wrote: > > that are private to XFS. What this means is that userspace should never > > be able to access any information about these files, and should not be > > able to open these files by handle. > > > > "Callers must not be allowed to link these files into the directory > > tree, which should suffice to keep these private inodes actually > > private. I_LINKABLE is therefore left unset. > > I_LINKABLE is only set for O_TMPFILE, so I wouldn't even bother with > that. But thinking about this: what i_nlink do these private inodes > have? If it is >= 1, we probably want to add an IS_PRIVATE check > to xfs_link just in case they ever leak out to a place where ->link > could be called. Ooh, that's a good catch. I'll check for IS_PRIVATE in xfs_vn_link. "We're about to start adding functionality that uses internal inodes that are private to XFS. What this means is that userspace should never be able to access any information about these files, and should not be able to open these files by handle. "To prevent userspace from ever finding the file, or mis-interactions with the security apparatus, set S_PRIVATE on the inode. Don't allow bulkstat, open-by-handle, or linking of S_PRIVATE files into the directory tree. This should keep private inodes actually private." --D
On Wed, Feb 28, 2024 at 09:33:25AM -0800, Darrick J. Wong wrote: > "We're about to start adding functionality that uses internal inodes > that are private to XFS. What this means is that userspace should never > be able to access any information about these files, and should not be > able to open these files by handle. > > "To prevent userspace from ever finding the file, or mis-interactions > with the security apparatus, set S_PRIVATE on the inode. Don't allow > bulkstat, open-by-handle, or linking of S_PRIVATE files into the > directory tree. This should keep private inodes actually private." Sounds good.
diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c index 7cd09c3a82cb5..4b03221351c0f 100644 --- a/fs/xfs/xfs_export.c +++ b/fs/xfs/xfs_export.c @@ -160,7 +160,7 @@ xfs_nfs_get_inode( } } - if (VFS_I(ip)->i_generation != generation) { + if (VFS_I(ip)->i_generation != generation || IS_PRIVATE(VFS_I(ip))) { xfs_irele(ip); return ERR_PTR(-ESTALE); } diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c index 14462614fcc8d..4610660f267e6 100644 --- a/fs/xfs/xfs_itable.c +++ b/fs/xfs/xfs_itable.c @@ -97,6 +97,14 @@ xfs_bulkstat_one_int( vfsuid = i_uid_into_vfsuid(idmap, inode); vfsgid = i_gid_into_vfsgid(idmap, inode); + /* If this is a private inode, don't leak its details to userspace. */ + if (IS_PRIVATE(inode)) { + xfs_iunlock(ip, XFS_ILOCK_SHARED); + xfs_irele(ip); + error = -EINVAL; + goto out_advance; + } + /* xfs_iget returns the following without needing * further change. */