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

["Darrick J. Wong" <djwong@kernel.org>]

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
>