[PATCH] xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()

[PATCH] xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()

[Dan Carpenter]

The "bufsize" comes from the root user.  If "bufsize" is negative then,
because of type promotion, neither of the validation checks at the start
of the function are able to catch it:

	if (bufsize < sizeof(struct xfs_attrlist) ||
	    bufsize > XFS_XATTR_LIST_MAX)
		return -EINVAL;

This means "bufsize" will trigger (WARN_ON_ONCE(size > INT_MAX)) in
kvmalloc_node().  Fix this by changing the type from int to size_t.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
It's sort of hard to figure out which Fixes tag to use...  Maybe:

Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")

so it gets backported to the kernels which have the warning?

 fs/xfs/xfs_ioctl.c | 2 +-
 fs/xfs/xfs_ioctl.h | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 174cd8950cb6..29231a8c8a45 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -372,7 +372,7 @@ int
 xfs_ioc_attr_list(
 	struct xfs_inode		*dp,
 	void __user			*ubuf,
-	int				bufsize,
+	size_t				bufsize,
 	int				flags,
 	struct xfs_attrlist_cursor __user *ucursor)
 {
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index 28453a6d4461..845d3bcab74b 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -38,8 +38,9 @@ xfs_readlink_by_handle(
 int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
 		uint32_t opcode, void __user *uname, void __user *value,
 		uint32_t *len, uint32_t flags);
-int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf, int bufsize,
-	int flags, struct xfs_attrlist_cursor __user *ucursor);
+int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf,
+		      size_t bufsize, int flags,
+		      struct xfs_attrlist_cursor __user *ucursor);
 
 extern struct dentry *
 xfs_handle_to_dentry(
-- 
2.20.1

Re: [PATCH] xfs: prevent a WARN_ONCE() in xfs_ioc_attr_list()

[Darrick J. Wong]

On Fri, Dec 17, 2021 at 09:54:53AM +0300, Dan Carpenter wrote:
> The "bufsize" comes from the root user.  If "bufsize" is negative then,
> because of type promotion, neither of the validation checks at the start
> of the function are able to catch it:
> 
> 	if (bufsize < sizeof(struct xfs_attrlist) ||
> 	    bufsize > XFS_XATTR_LIST_MAX)
> 		return -EINVAL;
> 
> This means "bufsize" will trigger (WARN_ON_ONCE(size > INT_MAX)) in
> kvmalloc_node().  Fix this by changing the type from int to size_t.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Makes sense, particularly since the only caller supplies a u32 anyway.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

> ---
> It's sort of hard to figure out which Fixes tag to use...  Maybe:
> 
> Fixes: 7661809d493b ("mm: don't allow oversized kvmalloc() calls")
> 
> so it gets backported to the kernels which have the warning?

But that's just the warning, right?  I think the root problem here is
turning the ioctl's u32 length argument into a signed int for parameter
passing, and then promoting it back to unsigned types for validation and
memory allocation.  I would have suggested:

Fixes: 3e7a779937a2 ("xfs: move the legacy xfs_attr_list to xfs_ioctl.c")

to trigger the autobackport robots, though that's a 2020 commit and
hence not so useful for spelunking.

Looking through the multiple reorganiziations of the git tree I think
the validation error itself dates to before 2013, but patching old
kernels will require the human touch.

--D

> 
>  fs/xfs/xfs_ioctl.c | 2 +-
>  fs/xfs/xfs_ioctl.h | 5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 174cd8950cb6..29231a8c8a45 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -372,7 +372,7 @@ int
>  xfs_ioc_attr_list(
>  	struct xfs_inode		*dp,
>  	void __user			*ubuf,
> -	int				bufsize,
> +	size_t				bufsize,
>  	int				flags,
>  	struct xfs_attrlist_cursor __user *ucursor)
>  {
> diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
> index 28453a6d4461..845d3bcab74b 100644
> --- a/fs/xfs/xfs_ioctl.h
> +++ b/fs/xfs/xfs_ioctl.h
> @@ -38,8 +38,9 @@ xfs_readlink_by_handle(
>  int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
>  		uint32_t opcode, void __user *uname, void __user *value,
>  		uint32_t *len, uint32_t flags);
> -int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf, int bufsize,
> -	int flags, struct xfs_attrlist_cursor __user *ucursor);
> +int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf,
> +		      size_t bufsize, int flags,
> +		      struct xfs_attrlist_cursor __user *ucursor);
>  
>  extern struct dentry *
>  xfs_handle_to_dentry(
> -- 
> 2.20.1
>