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@xxxxxxxxxx>
Makes sense, particularly since the only caller supplies a u32 anyway.
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> 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
>
