* [PATCH] btrfs-progs: common: make sure that qgroup id is in range
@ 2021-03-15 15:56 Sidong Yang
2021-03-15 18:22 ` David Sterba
2021-03-16 5:44 ` Qu Wenruo
0 siblings, 2 replies; 6+ messages in thread
From: Sidong Yang @ 2021-03-15 15:56 UTC (permalink / raw)
To: linux-btrfs, dsterba; +Cc: Sidong Yang
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 <realwakka@gmail.com>
---
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
2021-03-15 15:56 Sidong Yang
@ 2021-03-15 18:22 ` David Sterba
2021-03-16 5:44 ` Qu Wenruo
1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2021-03-15 18:22 UTC (permalink / raw)
To: Sidong Yang; +Cc: linux-btrfs, dsterba
On Mon, Mar 15, 2021 at 03:56:38PM +0000, 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 <realwakka@gmail.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
2021-03-15 15:56 Sidong Yang
2021-03-15 18:22 ` David Sterba
@ 2021-03-16 5:44 ` Qu Wenruo
2021-03-16 12:58 ` Sidong Yang
1 sibling, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2021-03-16 5:44 UTC (permalink / raw)
To: Sidong Yang, linux-btrfs, dsterba
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 <realwakka@gmail.com>
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;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
2021-03-16 5:44 ` Qu Wenruo
@ 2021-03-16 12:58 ` Sidong Yang
2021-03-16 13:03 ` Qu Wenruo
0 siblings, 1 reply; 6+ messages in thread
From: Sidong Yang @ 2021-03-16 12:58 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, dsterba
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 <realwakka@gmail.com>
>
> 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;
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: common: make sure that qgroup id is in range
2021-03-16 12:58 ` Sidong Yang
@ 2021-03-16 13:03 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2021-03-16 13:03 UTC (permalink / raw)
To: Sidong Yang; +Cc: linux-btrfs, dsterba
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 <realwakka@gmail.com>
>>
>> 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;
>>>
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] btrfs-progs: common: make sure that qgroup id is in range
@ 2021-03-19 17:09 Sidong Yang
0 siblings, 0 replies; 6+ messages in thread
From: Sidong Yang @ 2021-03-19 17:09 UTC (permalink / raw)
To: linux-btrfs, dsterba, Qu Wenruo; +Cc: Sidong Yang
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]. and also checks qgroup
level that is in range [0, 2^16-1].
Signed-off-by: Sidong Yang <realwakka@gmail.com>
--
v2:
Use btrfs_qgroup_level() for checking
v3:
Add checks for qgroup level
---
common/utils.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/common/utils.c b/common/utils.c
index 57e41432..69fa6096 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -708,6 +708,10 @@ u64 parse_size_from_string(const char *s)
return ret;
}
+static bool valid_qgroup_level(u64 level) {
+ return !(level >> (sizeof(level) * 8 - BTRFS_QGROUP_LEVEL_SHIFT));
+}
+
u64 parse_qgroupid(const char *p)
{
char *s = strchr(p, '/');
@@ -727,15 +731,23 @@ u64 parse_qgroupid(const char *p)
id = strtoull(p, &ptr_parse_end, 10);
if (ptr_parse_end != ptr_src_end)
goto path;
+ if (btrfs_qgroup_level(id))
+ goto err;
return id;
}
level = strtoull(p, &ptr_parse_end, 10);
if (ptr_parse_end != s)
goto path;
+ if (!valid_qgroup_level(level))
+ goto err;
+
id = strtoull(s + 1, &ptr_parse_end, 10);
if (ptr_parse_end != ptr_src_end)
- goto path;
+ goto path;
+
+ if (btrfs_qgroup_level(id))
+ goto err;
return (level << BTRFS_QGROUP_LEVEL_SHIFT) | id;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-19 17:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-19 17:09 [PATCH] btrfs-progs: common: make sure that qgroup id is in range Sidong Yang
-- strict thread matches above, loose matches on Subject: below --
2021-03-15 15:56 Sidong Yang
2021-03-15 18:22 ` David Sterba
2021-03-16 5:44 ` Qu Wenruo
2021-03-16 12:58 ` Sidong Yang
2021-03-16 13:03 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox