[5/4] xfs: fix negative array access in xfs_getbmap

Commit Message

Darrick J. Wong May 1, 2023, 9:24 p.m. UTC

From: Darrick J. Wong <[e-mail]>

In commit 8ee81ed581ff, Ye Bin complained about an ASSERT in the bmapx
code that trips if we encounter a delalloc extent after flushing the
pagecache to disk.  The ioctl code does not hold MMAPLOCK so it's
entirely possible that a racing write page fault can create a delalloc
extent after the file has been flushed.  The proposed solution was to
replace the assertion with an early return that avoids filling out the
bmap recordset with a delalloc entry if the caller didn't ask for it.

At the time, I recall thinking that the forward logic sounded ok, but
felt hesitant because I suspected that changing this code would cause
something /else/ to burst loose due to some other subtlety.

syzbot of course found that subtlety.  If all the extent mappings found
after the flush are delalloc mappings, we'll reach the end of the data
fork without ever incrementing bmv->bmv_entries.  This is new, since
before we'd have emitted the delalloc mappings even though the caller
didn't ask for them.  Once we reach the end, we'll try to set
BMV_OF_LAST on the -1st entry (because bmv_entries is zero) and go
corrupt something else in memory.  Yay.

I really dislike all these stupid patches that fiddle around with debug
code and break things that otherwise worked well enough.  Nobody was
complaining that calling XFS_IOC_BMAPX without BMV_IF_DELALLOC would
return BMV_OF_DELALLOC records, and now we've gone from "weird behavior
that nobody cared about" to "bad behavior that must be addressed
immediately".

Maybe I'll just ignore anything from Huawei from now on for my own sake.

Reported-by: [e-mail]
Link: https://lore.kernel.org/linux-xfs/20230412024907.GP360889@frogsfrogsfrogs/
Fixes: 8ee81ed581ff ("xfs: fix BUG_ON in xfs_getbmap()")
Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/xfs/xfs_bmap_util.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Dave Chinner May 1, 2023, 11:09 p.m. UTC | #1

On Mon, May 01, 2023 at 02:24:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> In commit 8ee81ed581ff, Ye Bin complained about an ASSERT in the bmapx
> code that trips if we encounter a delalloc extent after flushing the
> pagecache to disk.  The ioctl code does not hold MMAPLOCK so it's
> entirely possible that a racing write page fault can create a delalloc
> extent after the file has been flushed.  The proposed solution was to
> replace the assertion with an early return that avoids filling out the
> bmap recordset with a delalloc entry if the caller didn't ask for it.
> 
> At the time, I recall thinking that the forward logic sounded ok, but
> felt hesitant because I suspected that changing this code would cause
> something /else/ to burst loose due to some other subtlety.
> 
> syzbot of course found that subtlety.  If all the extent mappings found
> after the flush are delalloc mappings, we'll reach the end of the data
> fork without ever incrementing bmv->bmv_entries.  This is new, since
> before we'd have emitted the delalloc mappings even though the caller
> didn't ask for them.  Once we reach the end, we'll try to set
> BMV_OF_LAST on the -1st entry (because bmv_entries is zero) and go
> corrupt something else in memory.  Yay.
> 
> I really dislike all these stupid patches that fiddle around with debug
> code and break things that otherwise worked well enough.  Nobody was
> complaining that calling XFS_IOC_BMAPX without BMV_IF_DELALLOC would
> return BMV_OF_DELALLOC records, and now we've gone from "weird behavior
> that nobody cared about" to "bad behavior that must be addressed
> immediately".
> 
> Maybe I'll just ignore anything from Huawei from now on for my own sake.
> 
> Reported-by: [e-mail]
> Link: https://lore.kernel.org/linux-xfs/20230412024907.GP360889@frogsfrogsfrogs/
> Fixes: 8ee81ed581ff ("xfs: fix BUG_ON in xfs_getbmap()")
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  fs/xfs/xfs_bmap_util.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Ugh. Yet again we add weight to the approach of "if it ain't broke,
don't fix it" for maintaining code that has not changed for a long
time...

Reviewed-by: Dave Chinner <[e-mail]>

yebin (H) May 4, 2023, 12:43 p.m. UTC | #2

On 2023/5/2 5:24, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
>
> In commit 8ee81ed581ff, Ye Bin complained about an ASSERT in the bmapx
> code that trips if we encounter a delalloc extent after flushing the
> pagecache to disk.  The ioctl code does not hold MMAPLOCK so it's
> entirely possible that a racing write page fault can create a delalloc
> extent after the file has been flushed.  The proposed solution was to
> replace the assertion with an early return that avoids filling out the
> bmap recordset with a delalloc entry if the caller didn't ask for it.
>
> At the time, I recall thinking that the forward logic sounded ok, but
> felt hesitant because I suspected that changing this code would cause
> something /else/ to burst loose due to some other subtlety.
>
> syzbot of course found that subtlety.  If all the extent mappings found
> after the flush are delalloc mappings, we'll reach the end of the data
> fork without ever incrementing bmv->bmv_entries.  This is new, since
> before we'd have emitted the delalloc mappings even though the caller
> didn't ask for them.  Once we reach the end, we'll try to set
> BMV_OF_LAST on the -1st entry (because bmv_entries is zero) and go
> corrupt something else in memory.  Yay.
>
> I really dislike all these stupid patches that fiddle around with debug
> code and break things that otherwise worked well enough.  Nobody was
> complaining that calling XFS_IOC_BMAPX without BMV_IF_DELALLOC would
> return BMV_OF_DELALLOC records, and now we've gone from "weird behavior
> that nobody cared about" to "bad behavior that must be addressed
> immediately".
>
> Maybe I'll just ignore anything from Huawei from now on for my own sake.
I am very sorry for introducing a new issue and causing you inconvenience.
The issue fixed by commit 8ee81ed581ff was triggered by doing our syzkaller
testing，and my intention is to fix the issue without any malice and offend.

I fully agree with you that we should be more cautious in modifying the code
that was originally working well. I will do more self code review and 
test before
sending patches to upstream.
> Reported-by: [e-mail]
> Link: https://lore.kernel.org/linux-xfs/20230412024907.GP360889@frogsfrogsfrogs/
> Fixes: 8ee81ed581ff ("xfs: fix BUG_ON in xfs_getbmap()")
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>   fs/xfs/xfs_bmap_util.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index f032d3a4b727..fbb675563208 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -558,7 +558,9 @@ xfs_getbmap(
>   		if (!xfs_iext_next_extent(ifp, &icur, &got)) {
>   			xfs_fileoff_t	end = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
>   
> -			out[bmv->bmv_entries - 1].bmv_oflags |= BMV_OF_LAST;
> +			if (bmv->bmv_entries > 0)
> +				out[bmv->bmv_entries - 1].bmv_oflags |=
> +								BMV_OF_LAST;
>   
>   			if (whichfork != XFS_ATTR_FORK && bno < end &&
>   			    !xfs_getbmap_full(bmv)) {
> .
>

Patch

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index f032d3a4b727..fbb675563208 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -558,7 +558,9 @@  xfs_getbmap(
 		if (!xfs_iext_next_extent(ifp, &icur, &got)) {
 			xfs_fileoff_t	end = XFS_B_TO_FSB(mp, XFS_ISIZE(ip));
 
-			out[bmv->bmv_entries - 1].bmv_oflags |= BMV_OF_LAST;
+			if (bmv->bmv_entries > 0)
+				out[bmv->bmv_entries - 1].bmv_oflags |=
+								BMV_OF_LAST;
 
 			if (whichfork != XFS_ATTR_FORK && bno < end &&
 			    !xfs_getbmap_full(bmv)) {