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 <[email protected]> > > Reviewed-by: Josef Bacik <[email protected]> >
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 <[email protected]> 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); } _______________________________________________ stable mailing list [email protected] http://linux.kernel.org/mailman/listinfo/stable
