On 2018/9/8 上午4:50, Omar Sandoval wrote:
> On Fri, Aug 31, 2018 at 10:29:29AM +0800, Qu Wenruo wrote:
>> btrfs_qgroup_inherit structure doesn't goes through much validation
>> check.
>>
>> Now do a comprehensive check for it, including:
>> 1) inherit size
>> Should not exceeding SZ_4K and its num_qgroups should not
>> exceed its size passed in btrfs_ioctl_vol_args_v2.
>>
>> 2) flags
>> Should not include any unknown flags
>> (In fact, no flag is supported at all now)
>> Btrfs-progs never has such ability to set flags for btrfs_qgroup_inherit.
>>
>> 3) limit
>> Should not contain anything.
>> Btrfs-progs never has such ability to set limit for btrfs_qgroup_inherit.
>>
>> 4) rfer/excl copy
>> Deprecated feature.
>> Btrfs-progs has such interface but never documented and we're already
>> going to remove such ability.
>> It's the easiest way to screw up qgroup numbers.
>>
>> 3) Qgroupid
>> Comprehensive check is already in btrfs_qgroup_inherit(), here we
>> only check if there is any obviously invalid qgroupid (0).
>>
>> Coverity-id: 1021055
>> Reported-by: Nikolay Borisov <nborisov@xxxxxxxx>
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>> ---
>> fs/btrfs/ioctl.c | 3 +++
>> fs/btrfs/qgroup.c | 39 ++++++++++++++++++++++++++++++++++++++
>> fs/btrfs/qgroup.h | 2 ++
>> include/uapi/linux/btrfs.h | 17 ++++++++---------
>> 4 files changed, 52 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 5db8680b40a9..4f5f453d5d07 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1820,6 +1820,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
>> ret = PTR_ERR(inherit);
>> goto free_args;
>> }
>> + ret = btrfs_validate_inherit(inherit, vol_args->size);
>> + if (ret < 0)
>> + goto free_args;
>> }
>>
>> ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 4353bb69bb86..53daf73b0de9 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2232,6 +2232,45 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans)
>> return ret;
>> }
>>
>> +/*
>> + * To make sure the inherit passed in is valid
>> + *
>> + * Here we only check flags and rule out some no-longer supported features.
>> + * And we only do very basis qgroupid check to ensure there is no obviously
>> + * invalid qgroupid (0). Detailed qgroupid check will be done in
>> + * btrfs_qgroup_inherit().
>> + */
>> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
>> + u64 inherit_size)
>> +{
>> + u64 i;
>> +
>> + if (inherit->flags & ~BTRFS_QGROUP_INHERIT_FLAGS_SUPP)
>> + return -ENOTTY;
>> + /* Qgroup rfer/excl copy is deprecated */
>> + if (inherit->num_excl_copies || inherit->num_ref_copies)
>> + return -ENOTTY;
>> +
>> + /* Since SET_LIMITS is never used, @lim should all be zeroed */
>> + if (inherit->lim.max_excl || inherit->lim.max_rfer ||
>> + inherit->lim.rsv_excl || inherit->lim.rsv_rfer ||
>> + inherit->lim.flags)
>> + return -ENOTTY;
>
> Why -ENOTTY? I think these should all be -EINVAL.
Changed in v2.
Now it only outputs warning message.
>
>> + /* Size check */
>> + if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) >
>
> This arithmetic can overflow...
Oh, forgot that possibility.
inherit->num_qgroups/excl_copies/rfer_coopies should be checked against
(BTRFS_QGROUP_INHERIT_MAX_SIZE - sizeof(*inherit))/(sizeof(u64).
Thanks,
Qu
>
>> + min_t(u64, BTRFS_QGROUP_INHERIT_MAX_SIZE, inherit_size))
>> + return -EINVAL;
>> +
>> +
>> + /* Qgroup 0/0 is not allowed */
>> + for (i = 0; i < inherit->num_qgroups; i++) {
>> + if (inherit->qgroups[i] == 0)
>
> Which means we can access out of bounds here.
>
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> /*
>> * Copy the accounting information between qgroups. This is necessary
>> * when a snapshot or a subvolume is created. Throwing an error will
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index 54b8bb282c0e..1bf9c584be70 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -241,6 +241,8 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>> struct ulist *new_roots);
>> int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans);
>> int btrfs_run_qgroups(struct btrfs_trans_handle *trans);
>> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
>> + u64 inherit_size);
>> int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>> u64 objectid, struct btrfs_qgroup_inherit *inherit);
>> void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index 311edb65567c..5a5532a20019 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -74,21 +74,20 @@ struct btrfs_qgroup_limit {
>> __u64 rsv_excl;
>> };
>>
>> -/*
>> - * flags definition for qgroup inheritance
>> - *
>> - * Used by:
>> - * struct btrfs_qgroup_inherit.flags
>> - */
>> +/* flags definition for qgroup inheritance (DEPRECATED) */
>> #define BTRFS_QGROUP_INHERIT_SET_LIMITS (1ULL << 0)
>>
>> +/* No supported flags */
>> +#define BTRFS_QGROUP_INHERIT_FLAGS_SUPP (0)
>> +
>> #define BTRFS_QGROUP_INHERIT_MAX_SIZE (SZ_4K)
>> +
>> struct btrfs_qgroup_inherit {
>> __u64 flags;
>> __u64 num_qgroups;
>> - __u64 num_ref_copies;
>> - __u64 num_excl_copies;
>> - struct btrfs_qgroup_limit lim;
>> + __u64 num_ref_copies; /* DEPRECATED */
>> + __u64 num_excl_copies; /* DEPRECATED */
>> + struct btrfs_qgroup_limit lim; /* DEPRECATED */
>> __u64 qgroups[0];
>> };
>>
>> --
>> 2.18.0
>>
Attachment:
signature.asc
Description: OpenPGP digital signature
