[PATCH] xfs: remove file privileges after XFS_IOC_SWAPEXT

[PATCH] xfs: remove file privileges after XFS_IOC_SWAPEXT

[Qing Ming]

XFS_IOC_SWAPEXT exchanges the data forks of two regular files. This
changes file contents and therefore needs the same privilege stripping
that ordinary write paths apply.

The legacy ioctl currently completes the exchange without removing
SUID/SGID bits or file capabilities. As a result, a privileged inode can
retain those attributes after its data fork has been replaced.

Pass the file objects into xfs_swap_extents() and call file_remove_privs()
for both files after the exchange commits, before dropping the outer
inode and mapping locks. This matches the XFS_IOC_EXCHANGE_RANGE finish
path.

Signed-off-by: Qing Ming <a0yami@mailbox.org>
---
 fs/xfs/xfs_bmap_util.c | 18 ++++++++++++++++--
 fs/xfs/xfs_bmap_util.h |  3 ++-
 fs/xfs/xfs_ioctl.c     |  2 +-
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 0ab00615f..ced81309b 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1505,10 +1505,12 @@ xfs_swap_change_owner(
 
 int
 xfs_swap_extents(
-	struct xfs_inode	*ip,	/* target inode */
-	struct xfs_inode	*tip,	/* tmp inode */
+	struct file		*file,
+	struct file		*tmp_file,
 	struct xfs_swapext	*sxp)
 {
+	struct xfs_inode	*ip = XFS_I(file_inode(file));
+	struct xfs_inode	*tip = XFS_I(file_inode(tmp_file));
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_trans	*tp;
 	struct xfs_bstat	*sbp = &sxp->sx_stat;
@@ -1727,10 +1729,22 @@ xfs_swap_extents(
 		xfs_trans_set_sync(tp);
 
 	error = xfs_trans_commit(tp);
+	if (error)
+		goto out_unlock_ilock;
 
 	trace_xfs_swap_extent_after(ip, 0);
 	trace_xfs_swap_extent_after(tip, 1);
 
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	xfs_iunlock(tip, XFS_ILOCK_EXCL);
+
+	error = file_remove_privs(file);
+	if (error)
+		goto out_unlock;
+	if (file_inode(file) != file_inode(tmp_file))
+		error = file_remove_privs(tmp_file);
+	goto out_unlock;
+
 out_unlock_ilock:
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
 	xfs_iunlock(tip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
index c477b3361..a6043d1aa 100644
--- a/fs/xfs/xfs_bmap_util.h
+++ b/fs/xfs/xfs_bmap_util.h
@@ -8,6 +8,7 @@
 
 /* Kernel only BMAP related definitions and functions */
 
+struct file;
 struct xfs_bmbt_irec;
 struct xfs_extent_free_item;
 struct xfs_ifork;
@@ -68,7 +69,7 @@ int	xfs_insert_file_space(struct xfs_inode *, xfs_off_t offset,
 bool	xfs_can_free_eofblocks(struct xfs_inode *ip);
 int	xfs_free_eofblocks(struct xfs_inode *ip);
 
-int	xfs_swap_extents(struct xfs_inode *ip, struct xfs_inode *tip,
+int	xfs_swap_extents(struct file *file, struct file *tmp_file,
 			 struct xfs_swapext *sx);
 
 xfs_daddr_t xfs_fsb_to_db(struct xfs_inode *ip, xfs_fsblock_t fsb);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 46e234863..ea1047d6a 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -990,7 +990,7 @@ xfs_ioc_swapext(
 	if (xfs_is_shutdown(ip->i_mount))
 		return -EIO;
 
-	return xfs_swap_extents(ip, tip, sxp);
+	return xfs_swap_extents(fd_file(f), fd_file(tmp), sxp);
 }
 
 static int
-- 
2.53.0

Re: [PATCH] xfs: remove file privileges after XFS_IOC_SWAPEXT

[Christoph Hellwig]

On Sun, May 24, 2026 at 02:38:20PM +0800, Qing Ming wrote:
> XFS_IOC_SWAPEXT exchanges the data forks of two regular files. This
> changes file contents and therefore needs the same privilege stripping
> that ordinary write paths apply.
> 
> The legacy ioctl currently completes the exchange without removing
> SUID/SGID bits or file capabilities. As a result, a privileged inode can
> retain those attributes after its data fork has been replaced.
> 
> Pass the file objects into xfs_swap_extents() and call file_remove_privs()
> for both files after the exchange commits, before dropping the outer
> inode and mapping locks. This matches the XFS_IOC_EXCHANGE_RANGE finish
> path.

Not sure this makes much sense, as xfs_swap_extents is used for
defragmentation, including system-wide one, and this would drop the
suid bit from existing suid bit files and break the system.

I don't think there is a security issue as the target file needs to
be writable to the user performing the operation, and the owning uid/gid
has to match as well.

I think the issue is more that XFS_IOC_COMMIT_RANGE drops the suid
and could thus cause problems when used by fsr.