[PATCH] xfs: fix NULL pointer dereference in xfs_getbmap()

[PATCH] xfs: fix NULL pointer dereference in xfs_getbmap()

[ChenXiaoSong]

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().

Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
---
 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 74f96e1aa5cd..04d0c2bff67c 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_inode_has_attr_fork(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.31.1

Re: [PATCH] xfs: fix NULL pointer dereference in xfs_getbmap()

[Darrick J. Wong]

On Wed, Jul 27, 2022 at 04:52:30PM +0800, ChenXiaoSong wrote:
> 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);

Heh.  Is this worth an fstest?  It probably is, since prior to 5.20 this
would have been a UAF bug on top of a NULL deref.

> 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().

Nit: it's ILOCK, not i_lock.  Otherwise... this looks correct to me --
take the IOLOCK and ILOCK in shared mode before accessing the inode fork
structures.

Do you have any suggestions for Fixes:?  I suspect this has been broken
for quite some time.

No need to fix the nit, I'll do that when I commit this.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> Signed-off-by: ChenXiaoSong <chenxiaosong2@huawei.com>
> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
> ---
>  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 74f96e1aa5cd..04d0c2bff67c 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_inode_has_attr_fork(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.31.1
> 

Re: [PATCH] xfs: fix NULL pointer dereference in xfs_getbmap()

[chenxiaosong (A)]

在 2022/7/27 23:16, Darrick J. Wong 写道:

> Heh.  Is this worth an fstest?  It probably is, since prior to 5.20 this
> would have been a UAF bug on top of a NULL deref.

I will try to add regression test to xfstests for this commit when it is 
merged into mainline.

> 
> Nit: it's ILOCK, not i_lock.  Otherwise... this looks correct to me --
> take the IOLOCK and ILOCK in shared mode before accessing the inode fork
> structures.
> 
> Do you have any suggestions for Fixes:?  I suspect this has been broken
> for quite some time.

The "Fixes:" label is: abbf9e8a4507 ("xfs: rewrite getbmap using the 
xfs_iext_* helpers")

> 
> No need to fix the nit, I'll do that when I commit this.
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
>