lkml.org 
[lkml]   [2010]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
    From
    Thanks for your help, i'll send a new patch soon.

    On Wed, Mar 17, 2010 at 10:41 AM, Dave Chinner <david@fromorbit.com> wrote:
    > On Tue, Mar 16, 2010 at 11:53:50PM +0800, wzt.wzt@gmail.com wrote:
    >> STATIC int
    >> xfs_compat_attrmulti_by_handle(
    >>         struct file                             *parfilp,
    >>         void                                    __user *arg)
    >> {
    >> ...
    >>         if (copy_from_user(&am_hreq, arg,
    >>                            sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
    >>                 return -XFS_ERROR(EFAULT);
    >> ...
    >>         error = E2BIG;
    >>         /* Not check the am_hreq.opcount max value from userspace,
    >>         m_hreq.opcount * sizeof(compat_xfs_attr_multiop_t) can make
    >>         integer overflow, and the if condition can be bypass. Though,
    >>         it can not make security problem, but fix it maybe better. */
    >>         size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t);
    >>         if (!size || size > 16 * PAGE_SIZE)
    >>                 goto out_dput;
    >> ...
    >> }
    >
    > This description could use a little work. ;)
    >
    > Perhaps something like:
    >
    > The am_hreq.opcount field in the xfs_attrmulti_by_handle() interface
    > is not bounded correctly. The opcount is used to determine the size
    > of the buffer required. The size is bounded, but can overflow and so
    > the size checks may not be sufficient to catch invalid opcounts.
    > Fix it by catching opcount values that would cause overflows before
    > calculating the size.
    >
    >
    >>
    >> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
    >>
    >> ---
    >>  fs/xfs/linux-2.6/xfs_ioctl.c   |    4 ++++
    >>  fs/xfs/linux-2.6/xfs_ioctl32.c |    4 ++++
    >>  2 files changed, 8 insertions(+), 0 deletions(-)
    >>
    >> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
    >> index 4ea1ee1..b05b3b7 100644
    >> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
    >> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
    >> @@ -526,6 +526,10 @@ xfs_attrmulti_by_handle(
    >>       if (copy_from_user(&am_hreq, arg, sizeof(xfs_fsop_attrmulti_handlereq_t)))
    >>               return -XFS_ERROR(EFAULT);
    >>
    >> +     /* overflow check */
    >> +     if (am_hreq.opcount >= INT_MAX / sizeof(xfs_attr_multiop_t))
    >> +             return -ENOMEM;
    >> +
    >
    > The code currently return E2BIG for an opcount that is too large.
    > I think this should also return E2BIG rather then ENOMEM. Does that
    > seem reasonable?
    >
    > Cheers,
    >
    > Dave.
    > --
    > Dave Chinner
    > david@fromorbit.com
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-03-17 04:11    [W:0.027 / U:1.132 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site