Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
On 2021/3/16 下午8:58, Sidong Yang wrote: On Tue, Mar 16, 2021 at 01:44:33PM +0800, Qu Wenruo wrote: On 2021/3/15 下午11:56, Sidong Yang wrote: When user assign qgroup with qgroup id that is too big to exceeds range and invade level value, and it works without any error. but this action would be make undefined error. this code make sure that qgroup id doesn't exceed range(0 ~ 2^48-1). Signed-off-by: Sidong Yang Shouldn't the check also happen inside the ioctl? Yes, I checked the ioctl code in kernel. but there is only the code that check if it is zero like !sa->qgroupid. and it just assign to key.offset. Also it should be checked in ioctl? After more check, the ioctl interface doesn't need that check, or user can't parse any qgroup with higher qgroup level. Thus the check should only exist in user space to avoid case like 1/(U48_MAX + 1). Thanks, Qu Thanks, Qu --- common/utils.c | 5 + 1 file changed, 5 insertions(+) diff --git a/common/utils.c b/common/utils.c index 57e41432..a2f72550 100644 --- a/common/utils.c +++ b/common/utils.c @@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p) id = strtoull(p, &ptr_parse_end, 10); if (ptr_parse_end != ptr_src_end) goto path; + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) + goto err; return id; } level = strtoull(p, &ptr_parse_end, 10); @@ -734,6 +736,9 @@ u64 parse_qgroupid(const char *p) goto path; id = strtoull(s + 1, &ptr_parse_end, 10); + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) + goto err; + if (ptr_parse_end != ptr_src_end) goto path;
Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
On Tue, Mar 16, 2021 at 01:44:33PM +0800, Qu Wenruo wrote: > > > On 2021/3/15 下午11:56, Sidong Yang wrote: > > When user assign qgroup with qgroup id that is too big to exceeds > > range and invade level value, and it works without any error. but > > this action would be make undefined error. this code make sure that > > qgroup id doesn't exceed range(0 ~ 2^48-1). > > > > Signed-off-by: Sidong Yang > > Shouldn't the check also happen inside the ioctl? Yes, I checked the ioctl code in kernel. but there is only the code that check if it is zero like !sa->qgroupid. and it just assign to key.offset. Also it should be checked in ioctl? > > Thanks, > Qu > > --- > > common/utils.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/common/utils.c b/common/utils.c > > index 57e41432..a2f72550 100644 > > --- a/common/utils.c > > +++ b/common/utils.c > > @@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p) > > id = strtoull(p, &ptr_parse_end, 10); > > if (ptr_parse_end != ptr_src_end) > > goto path; > > + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) > > + goto err; > > return id; > > } > > level = strtoull(p, &ptr_parse_end, 10); > > @@ -734,6 +736,9 @@ u64 parse_qgroupid(const char *p) > > goto path; > > > > id = strtoull(s + 1, &ptr_parse_end, 10); > > + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) > > + goto err; > > + > > if (ptr_parse_end != ptr_src_end) > > goto path; > > > >
Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
On 2021/3/15 下午11:56, Sidong Yang wrote: When user assign qgroup with qgroup id that is too big to exceeds range and invade level value, and it works without any error. but this action would be make undefined error. this code make sure that qgroup id doesn't exceed range(0 ~ 2^48-1). Signed-off-by: Sidong Yang Shouldn't the check also happen inside the ioctl? Thanks, Qu --- common/utils.c | 5 + 1 file changed, 5 insertions(+) diff --git a/common/utils.c b/common/utils.c index 57e41432..a2f72550 100644 --- a/common/utils.c +++ b/common/utils.c @@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p) id = strtoull(p, &ptr_parse_end, 10); if (ptr_parse_end != ptr_src_end) goto path; + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) + goto err; return id; } level = strtoull(p, &ptr_parse_end, 10); @@ -734,6 +736,9 @@ u64 parse_qgroupid(const char *p) goto path; id = strtoull(s + 1, &ptr_parse_end, 10); + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) + goto err; + if (ptr_parse_end != ptr_src_end) goto path;
Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
On Mon, Mar 15, 2021 at 03:56:38PM +, Sidong Yang wrote: > When user assign qgroup with qgroup id that is too big to exceeds > range and invade level value, and it works without any error. but > this action would be make undefined error. this code make sure that > qgroup id doesn't exceed range(0 ~ 2^48-1). > > Signed-off-by: Sidong Yang > --- > common/utils.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/common/utils.c b/common/utils.c > index 57e41432..a2f72550 100644 > --- a/common/utils.c > +++ b/common/utils.c > @@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p) > id = strtoull(p, &ptr_parse_end, 10); > if (ptr_parse_end != ptr_src_end) > goto path; > + if (id >> BTRFS_QGROUP_LEVEL_SHIFT) > + goto err; Please add a helper that validates the id or use btrfs_qgroup_level and check that level is 0 if that makes sense in the context of the call.
