xfs
[Top] [All Lists]

Re: [patch] xfs: underflow bug in xfs_attrlist_by_handle()

To: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Subject: Re: [patch] xfs: underflow bug in xfs_attrlist_by_handle()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 1 Nov 2013 08:34:56 +1100
Cc: Ben Myers <bpm@xxxxxxx>, Fabian Yamaguchi <fabs@xxxxxxxxx>, security@xxxxxxxxxx, Alex Elder <elder@xxxxxxxxxx>, Nico Golde <nico@xxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131031180010.GA24839@xxxxxxxxxxxxxxxxx>
References: <20131025144452.GA28451@xxxxxxxxx> <20131031180010.GA24839@xxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Oct 31, 2013 at 09:00:10PM +0300, Dan Carpenter wrote:
> If we allocate less than sizeof(struct attrlist) then we end up
> corrupting memory or doing a ZERO_PTR_SIZE dereference.
> 
> This can only be triggered with CAP_SYS_ADMIN.
> 
> Reported-by: Nico Golde <nico@xxxxxxxxx>
> Reported-by: Fabian Yamaguchi <fabs@xxxxxxxxx>
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 4d61340..33ad9a7 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -442,7 +442,8 @@ xfs_attrlist_by_handle(
>               return -XFS_ERROR(EPERM);
>       if (copy_from_user(&al_hreq, arg, 
> sizeof(xfs_fsop_attrlist_handlereq_t)))
>               return -XFS_ERROR(EFAULT);
> -     if (al_hreq.buflen > XATTR_LIST_MAX)
> +     if (al_hreq.buflen < sizeof(struct attrlist) ||
> +         al_hreq.buflen > XATTR_LIST_MAX)
>               return -XFS_ERROR(EINVAL);

Yup, that's not checked in xfs_attr_list(). Looks like these are the
only direct callers of xfs_attr_list(), and the other callers of
xfs_attr_list_int() don't appear to have the same issue. Good find!

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>