On Mon, Nov 20, 2023 at 04:38:24PM +0100, Greg KH wrote:
On Wed, Nov 15, 2023 at 06:28:25PM -0800, Leah Rumancik wrote:From: ChenXiaoSong <chenxiaosong2@xxxxxxxxxx> [ Upstream commit 001c179c4e26d04db8c9f5e3fef9558b58356be6 ] Reproducer: 1. fallocate -l 100M image 2. mkfs.xfs -f image 3. mount image /mnt 4. setxattr("/mnt", "trusted.overlay.upper", NULL, 0, XATTR_CREATE) 5. char arg[32] = "\x01\xff\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00" "\x00\x00\x00\x00\x00\x08\x00\x00\x00\xc6\x2a\xf7"; fd = open("/mnt", O_RDONLY|O_DIRECTORY); ioctl(fd, _IOC(_IOC_READ|_IOC_WRITE, 0x58, 0x2c, 0x20), arg); NULL pointer dereference will occur when race happens between xfs_getbmap() and xfs_bmap_set_attrforkoff(): ioctl | setxattr ----------------------------|--------------------------- xfs_getbmap | xfs_ifork_ptr | xfs_inode_has_attr_fork | ip->i_forkoff == 0 | return NULL | ifp == NULL | | xfs_bmap_set_attrforkoff | ip->i_forkoff > 0 xfs_inode_has_attr_fork | ip->i_forkoff > 0 | ifp == NULL | ifp->if_format | Fix this by locking i_lock before xfs_ifork_ptr(). Fixes: abbf9e8a4507 ("xfs: rewrite getbmap using the xfs_iext_* helpers") Signed-off-by: ChenXiaoSong <chenxiaosong2@xxxxxxxxxx> Signed-off-by: Guo Xuenan <guoxuenan@xxxxxxxxxx> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> [djwong: added fixes tag] Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> Signed-off-by: Leah Rumancik <leah.rumancik@xxxxxxxxx> Acked-by: Chandan Babu R <chandanbabu@xxxxxxxxxx> --- fs/xfs/xfs_bmap_util.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index fd2ad6a3019c..bea6cc26abf9 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -439,29 +439,28 @@ xfs_getbmap( whichfork = XFS_COW_FORK; else whichfork = XFS_DATA_FORK; - ifp = XFS_IFORK_PTR(ip, whichfork); xfs_ilock(ip, XFS_IOLOCK_SHARED); switch (whichfork) { case XFS_ATTR_FORK: + lock = xfs_ilock_attr_map_shared(ip); if (!XFS_IFORK_Q(ip)) - goto out_unlock_iolock; + goto out_unlock_ilock; max_len = 1LL << 32; - lock = xfs_ilock_attr_map_shared(ip); break; case XFS_COW_FORK: + lock = XFS_ILOCK_SHARED; + xfs_ilock(ip, lock); + /* No CoW fork? Just return */ - if (!ifp) - goto out_unlock_iolock; + if (!XFS_IFORK_PTR(ip, whichfork)) + goto out_unlock_ilock; if (xfs_get_cowextsz_hint(ip)) max_len = mp->m_super->s_maxbytes; else max_len = XFS_ISIZE(ip); - - lock = XFS_ILOCK_SHARED; - xfs_ilock(ip, lock); break; case XFS_DATA_FORK: if (!(iflags & BMV_IF_DELALLOC) && @@ -491,6 +490,8 @@ xfs_getbmap( break; } + ifp = XFS_IFORK_PTR(ip, whichfork); + switch (ifp->if_format) { case XFS_DINODE_FMT_EXTENTS: case XFS_DINODE_FMT_BTREE: -- 2.43.0.rc0.421.g78406f8d94-googThis patch breaks the build, how was it tested? fs/xfs/xfs_bmap_util.c: In function ‘xfs_getbmap’: fs/xfs/xfs_bmap_util.c:457:21: error: the comparison will always evaluate as ‘true’ for the address of ‘i_df’ will never be NULL [-Werror=address] 457 | if (!XFS_IFORK_PTR(ip, whichfork)) | ^ In file included from fs/xfs/xfs_bmap_util.c:16: fs/xfs/xfs_inode.h:38:33: note: ‘i_df’ declared here 38 | struct xfs_ifork i_df; /* data fork */ | ^~~~ cc1: all warnings being treated as errors
That's odd. I actually ended up queueing these patches earlier, and I don't see any such warnings. Looking at the code, this is a bit weird too - do you see these warnings with the current 5.15 queue? -- Thanks, Sasha
