| Message ID | 20170317205120.GE16505@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Mar 17, 2017 at 11:51:20PM +0300, Dan Carpenter wrote: > This isn't super serious because you need CAP_ADMIN to run this code. > > I added this integer overflow check last year but apparently I am > rubbish at writing integer overflow checks... There are two issues. > First, access_ok() works on unsigned long type and not u64 so on 32 bit > systems the access_ok() could be checking a truncated size. The other > issue is that we should be using a stricter limit so we don't overflow > the kzalloc() setting ctx->clone_roots later in the function after the > access_ok(): > > alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1); > sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN); > > Fixes: f5ecec3ce21f ("btrfs: send: silence an integer overflow warning") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: David Sterba <dsterba@suse.com> I'll copy parts of the changelog and add as comments, it's not obvious what the check does. -- 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
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index 030d592ed1fe..ad9508e67384 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -6306,7 +6306,7 @@ long btrfs_ioctl_send(struct file *mnt_file, void __user *arg_) } if (arg->clone_sources_count > - ULLONG_MAX / sizeof(*arg->clone_sources)) { + ULONG_MAX / sizeof(struct clone_root) - 1) { ret = -EINVAL; goto out; }
This isn't super serious because you need CAP_ADMIN to run this code. I added this integer overflow check last year but apparently I am rubbish at writing integer overflow checks... There are two issues. First, access_ok() works on unsigned long type and not u64 so on 32 bit systems the access_ok() could be checking a truncated size. The other issue is that we should be using a stricter limit so we don't overflow the kzalloc() setting ctx->clone_roots later in the function after the access_ok(): alloc_size = sizeof(struct clone_root) * (arg->clone_sources_count + 1); sctx->clone_roots = kzalloc(alloc_size, GFP_KERNEL | __GFP_NOWARN); Fixes: f5ecec3ce21f ("btrfs: send: silence an integer overflow warning") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- 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