| Message ID | 20180831022930.3465-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit() | expand |
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@suse.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > 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. > + /* Size check */ > + if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) > This arithmetic can overflow... > + 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 >
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@suse.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> 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 >>
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; + + /* Size check */ + if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) > + 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) + 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]; };
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@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- 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(-)