lkml.org 
[lkml]   [2011]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] btrfs: prevent heap corruption in btrfs_ioctl_space_info()
On Wed, Feb 09, 2011 at 10:51:33AM -0500, Josef Bacik wrote:
> On Wed, Feb 09, 2011 at 09:12:46AM -0500, Dan Rosenberg wrote:
> > Commit bf5fc093c5b625e4259203f1cee7ca73488a5620 refactored
> > btrfs_ioctl_space_info() and introduced several security issues.
> >
> > space_args.space_slots is an unsigned 64-bit type controlled by a
> > possibly unprivileged caller. The comparison as a signed int type
> > allows providing values that are treated as negative and cause the
> > subsequent allocation size calculation to wrap, or be truncated to 0.
> > By providing a size that's truncated to 0, kmalloc() will return
> > ZERO_SIZE_PTR. It's also possible to provide a value smaller than the
> > slot count. The subsequent loop ignores the allocation size when
> > copying data in, resulting in a heap overflow or write to ZERO_SIZE_PTR.
> >
> > The fix changes the slot count type and comparison typecast to u64,
> > which prevents truncation or signedness errors, and also ensures that we
> > don't copy more data than we've allocated in the subsequent loop. Note
> > that zero-size allocations are no longer possible since there is already
> > an explicit check for space_args.space_slots being 0 and truncation of
> > this value is no longer an issue.
> >
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
>
> Reviewed-by: Josef Bacik <josef@redhat.com>
>

Argh sorry I take it back, this is wrong, we can have multiple raid types per
space info, so you need to put the slot_count-- in the inner loop farther down
to count the actual slots we're adding. Thanks,

Signed-off-by: Josef Bacik <josef@redhat.com>

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b72520b..89bfd41 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2203,7 +2203,7 @@ long btrfs_ioctl_space_info(struct btrfs_root *root, void __user *arg)
int num_types = 4;
int alloc_size;
int ret = 0;
- int slot_count = 0;
+ u64 slot_count = 0;
int i, c;

if (copy_from_user(&space_args,
@@ -2242,7 +2242,7 @@ long btrfs_ioctl_space_info(struct btrfs_root *root, void __user *arg)
goto out;
}

- slot_count = min_t(int, space_args.space_slots, slot_count);
+ slot_count = min_t(u64, space_args.space_slots, slot_count);

alloc_size = sizeof(*dest) * slot_count;

@@ -2262,6 +2262,9 @@ long btrfs_ioctl_space_info(struct btrfs_root *root, void __user *arg)
for (i = 0; i < num_types; i++) {
struct btrfs_space_info *tmp;

+ if (!slot_count)
+ break;
+
info = NULL;
rcu_read_lock();
list_for_each_entry_rcu(tmp, &root->fs_info->space_info,
@@ -2283,7 +2286,10 @@ long btrfs_ioctl_space_info(struct btrfs_root *root, void __user *arg)
memcpy(dest, &space, sizeof(space));
dest++;
space_args.total_spaces++;
+ slot_count--;
}
+ if (!slot_count)
+ break;
}
up_read(&info->groups_sem);
}

\
 
 \ /
  Last update: 2011-02-09 17:19    [W:0.046 / U:12.992 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site