[PATCHSET 0/1] xfs: tighten GETBMAP input validation

[PATCHSET 0/1] xfs: tighten GETBMAP input validation

[Darrick J. Wong]

Hi all,

syzbot complained that the input validation for XFS_IOC_GETBMAP isn't as
strict as the one in the kvmalloc calls that it uses to stage the bmap
reporting.  This is an annoying use of maintainer time, but fix it anyway.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=getbmap-validation-5.17
---
 fs/xfs/xfs_ioctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH 1/1] xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Syzbot tripped over the following complaint from the kernel:

WARNING: CPU: 2 PID: 15402 at mm/util.c:597 kvmalloc_node+0x11e/0x125 mm/util.c:597

While trying to run XFS_IOC_GETBMAP against the following structure:

struct getbmap fubar = {
	.bmv_count	= 0x22dae649,
};

Obviously, this is a crazy huge value since the next thing that the
ioctl would do is allocate 37GB of memory.  This is enough to make
kvmalloc mad, but isn't large enough to trip the validation functions.
In other words, I'm fussing with checks that were **already sufficient**
because that's easier than dealing with 644 internal bug reports.  Yes,
that's right, six hundred and forty-four.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_ioctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 03a6198c97f6..2515fe8299e1 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1464,7 +1464,7 @@ xfs_ioc_getbmap(
 
 	if (bmx.bmv_count < 2)
 		return -EINVAL;
-	if (bmx.bmv_count > ULONG_MAX / recsize)
+	if (bmx.bmv_count >= INT_MAX / recsize)
 		return -ENOMEM;
 
 	buf = kvcalloc(bmx.bmv_count, sizeof(*buf), GFP_KERNEL);

Re: [PATCH 1/1] xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*

[Alli]

On Tue, 2022-01-25 at 18:18 -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Syzbot tripped over the following complaint from the kernel:
> 
> WARNING: CPU: 2 PID: 15402 at mm/util.c:597 kvmalloc_node+0x11e/0x125
> mm/util.c:597
> 
> While trying to run XFS_IOC_GETBMAP against the following structure:
> 
> struct getbmap fubar = {
> 	.bmv_count	= 0x22dae649,
> };
> 
> Obviously, this is a crazy huge value since the next thing that the
> ioctl would do is allocate 37GB of memory.  This is enough to make
> kvmalloc mad, but isn't large enough to trip the validation
> functions.
> In other words, I'm fussing with checks that were **already
> sufficient**
> because that's easier than dealing with 644 internal bug
> reports.  Yes,
> that's right, six hundred and forty-four.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
This fix looks fine to me.  Lets get it through.  Thanks!
Reviewed-By: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 03a6198c97f6..2515fe8299e1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1464,7 +1464,7 @@ xfs_ioc_getbmap(
>  
>  	if (bmx.bmv_count < 2)
>  		return -EINVAL;
> -	if (bmx.bmv_count > ULONG_MAX / recsize)
> +	if (bmx.bmv_count >= INT_MAX / recsize)
>  		return -ENOMEM;
>  
>  	buf = kvcalloc(bmx.bmv_count, sizeof(*buf), GFP_KERNEL);
> 

Re: [PATCH 1/1] xfs: reject crazy array sizes being fed to XFS_IOC_GETBMAP*

[Catherine Hoang]

> On Jan 25, 2022, at 6:18 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Syzbot tripped over the following complaint from the kernel:
> 
> WARNING: CPU: 2 PID: 15402 at mm/util.c:597 kvmalloc_node+0x11e/0x125 mm/util.c:597
> 
> While trying to run XFS_IOC_GETBMAP against the following structure:
> 
> struct getbmap fubar = {
> 	.bmv_count	= 0x22dae649,
> };
> 
> Obviously, this is a crazy huge value since the next thing that the
> ioctl would do is allocate 37GB of memory.  This is enough to make
> kvmalloc mad, but isn't large enough to trip the validation functions.
> In other words, I'm fussing with checks that were **already sufficient**
> because that's easier than dealing with 644 internal bug reports.  Yes,
> that's right, six hundred and forty-four.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> fs/xfs/xfs_ioctl.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 

Looks good,
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 03a6198c97f6..2515fe8299e1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1464,7 +1464,7 @@ xfs_ioc_getbmap(
> 
> 	if (bmx.bmv_count < 2)
> 		return -EINVAL;
> -	if (bmx.bmv_count > ULONG_MAX / recsize)
> +	if (bmx.bmv_count >= INT_MAX / recsize)
> 		return -ENOMEM;
> 
> 	buf = kvcalloc(bmx.bmv_count, sizeof(*buf), GFP_KERNEL);
>