Btrfs devel - Feb 2011 - [PATCH] btrfs: prevent heap corruption in btrfs_ioctl_space_info()

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>
---
 fs/btrfs/ioctl.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 02d224e..f1a43df 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2208,7 +2208,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,
@@ -2247,7 +2247,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;
 
@@ -2267,6 +2267,12 @@ long btrfs_ioctl_space_info(struct btrfs_root *root, void
__user *arg)
 	for (i = 0; i < num_types; i++) {
 		struct btrfs_space_info *tmp;
 
+		/* Don''t copy in more than we allocated */
+		if (!slot_count)
+			break;
+
+		slot_count--;
+
 		info = NULL;
 		rcu_read_lock();
 		list_for_each_entry_rcu(tmp, &root->fs_info->space_info,

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>

Thanks,

Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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);
 	}

> 
> 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,
> 
Good catch, thanks.

Reviewed-by: Dan Rosenberg <drosenberg@vsecurity.com>

-Dan

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html