* [PATCH v2 0/3] Check content after reading from quota file
@ 2022-09-22 13:03 Zhihao Cheng
2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:03 UTC (permalink / raw)
To: jack, tytso, brauner
Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3
1. Fix invalid memory access of dquot.
2. Cleanup, replace places of block number checking with helper
function.
3. Add more sanity checking for the content read from quota file.
v1->v2:
Add CC-stable tag in first patch.
Rename check_free_block() -> check_dquot_block_header().
Zhihao Cheng (3):
quota: Check next/prev free block number after reading from quota file
quota: Replace all block number checking with helper function
quota: Add more checking after reading from quota file
fs/quota/quota_tree.c | 81 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 69 insertions(+), 12 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/3] quota: Check next/prev free block number after reading from quota file
2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
@ 2022-09-22 13:03 ` Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
2 siblings, 0 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:03 UTC (permalink / raw)
To: jack, tytso, brauner
Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3
Following process:
Init: v2_read_file_info: <3> dqi_free_blk 0 dqi_free_entry 5 dqi_blks 6
Step 1. chown bin f_a -> dquot_acquire -> v2_write_dquot:
qtree_write_dquot
do_insert_tree
find_free_dqentry
get_free_dqblk
write_blk(info->dqi_blocks) // info->dqi_blocks = 6, failure. The
content in physical block (corresponding to blk 6) is random.
Step 2. chown root f_a -> dquot_transfer -> dqput_all -> dqput ->
ext4_release_dquot -> v2_release_dquot -> qtree_delete_dquot:
dquot_release
remove_tree
free_dqentry
put_free_dqblk(6)
info->dqi_free_blk = blk // info->dqi_free_blk = 6
Step 3. drop cache (buffer head for block 6 is released)
Step 4. chown bin f_b -> dquot_acquire -> commit_dqblk -> v2_write_dquot:
qtree_write_dquot
do_insert_tree
find_free_dqentry
get_free_dqblk
dh = (struct qt_disk_dqdbheader *)buf
blk = info->dqi_free_blk // 6
ret = read_blk(info, blk, buf) // The content of buf is random
info->dqi_free_blk = le32_to_cpu(dh->dqdh_next_free) // random blk
Step 5. chown bin f_c -> notify_change -> ext4_setattr -> dquot_transfer:
dquot = dqget -> acquire_dquot -> ext4_acquire_dquot -> dquot_acquire ->
commit_dqblk -> v2_write_dquot -> dq_insert_tree:
do_insert_tree
find_free_dqentry
get_free_dqblk
blk = info->dqi_free_blk // If blk < 0 and blk is not an error
code, it will be returned as dquot
transfer_to[USRQUOTA] = dquot // A random negative value
__dquot_transfer(transfer_to)
dquot_add_inodes(transfer_to[cnt])
spin_lock(&dquot->dq_dqb_lock) // page fault
, which will lead to kernel page fault:
Quota error (device sda): qtree_write_dquot: Error -8000 occurred
while creating quota
BUG: unable to handle page fault for address: ffffffffffffe120
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
Oops: 0002 [#1] PREEMPT SMP
CPU: 0 PID: 5974 Comm: chown Not tainted 6.0.0-rc1-00004
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
RIP: 0010:_raw_spin_lock+0x3a/0x90
Call Trace:
dquot_add_inodes+0x28/0x270
__dquot_transfer+0x377/0x840
dquot_transfer+0xde/0x540
ext4_setattr+0x405/0x14d0
notify_change+0x68e/0x9f0
chown_common+0x300/0x430
__x64_sys_fchownat+0x29/0x40
In order to avoid accessing invalid quota memory address, this patch adds
block number checking of next/prev free block read from quota file.
Fetch a reproducer in [Link].
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216372
Fixes: 1da177e4c3f4152 ("Linux-2.6.12-rc2")
CC: stable@vger.kernel.org
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
fs/quota/quota_tree.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index 5f2405994280..f89186b6db1d 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,6 +71,35 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
return ret;
}
+static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
+{
+ if (val >= max_val) {
+ quota_error(sb, "Getting block too big (%u >= %u)",
+ val, max_val);
+ return -EUCLEAN;
+ }
+
+ return 0;
+}
+
+static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
+ struct qt_disk_dqdbheader *dh)
+{
+ int err = 0;
+ uint nextblk, prevblk;
+
+ nextblk = le32_to_cpu(dh->dqdh_next_free);
+ err = do_check_range(info->dqi_sb, nextblk, info->dqi_blocks);
+ if (err)
+ return err;
+ prevblk = le32_to_cpu(dh->dqdh_prev_free);
+ err = do_check_range(info->dqi_sb, prevblk, info->dqi_blocks);
+ if (err)
+ return err;
+
+ return err;
+}
+
/* Remove empty block from list and return it */
static int get_free_dqblk(struct qtree_mem_dqinfo *info)
{
@@ -85,6 +114,9 @@ static int get_free_dqblk(struct qtree_mem_dqinfo *info)
ret = read_blk(info, blk, buf);
if (ret < 0)
goto out_buf;
+ ret = check_dquot_block_header(info, dh);
+ if (ret)
+ goto out_buf;
info->dqi_free_blk = le32_to_cpu(dh->dqdh_next_free);
}
else {
@@ -232,6 +264,9 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
*err = read_blk(info, blk, buf);
if (*err < 0)
goto out_buf;
+ *err = check_dquot_block_header(info, dh);
+ if (*err)
+ goto out_buf;
} else {
blk = get_free_dqblk(info);
if ((int)blk < 0) {
@@ -424,6 +459,9 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
goto out_buf;
}
dh = (struct qt_disk_dqdbheader *)buf;
+ ret = check_dquot_block_header(info, dh);
+ if (ret)
+ goto out_buf;
le16_add_cpu(&dh->dqdh_entries, -1);
if (!le16_to_cpu(dh->dqdh_entries)) { /* Block got free? */
ret = remove_free_dqentry(info, buf, blk);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] quota: Replace all block number checking with helper function
2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
@ 2022-09-22 13:04 ` Zhihao Cheng
2022-09-23 11:48 ` Jan Kara
2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
2 siblings, 1 reply; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:04 UTC (permalink / raw)
To: jack, tytso, brauner
Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3
Cleanup all block checking places, replace them with helper function
do_check_range().
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
fs/quota/quota_tree.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index f89186b6db1d..47711e739ddb 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
return ret;
}
-static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
+static inline int do_check_range(struct super_block *sb, uint val,
+ uint min_val, uint max_val)
{
- if (val >= max_val) {
- quota_error(sb, "Getting block too big (%u >= %u)",
- val, max_val);
+ if (val < min_val || val >= max_val) {
+ quota_error(sb, "Getting block %u out of range %u-%u",
+ val, min_val, max_val);
return -EUCLEAN;
}
@@ -89,11 +90,11 @@ static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
uint nextblk, prevblk;
nextblk = le32_to_cpu(dh->dqdh_next_free);
- err = do_check_range(info->dqi_sb, nextblk, info->dqi_blocks);
+ err = do_check_range(info->dqi_sb, nextblk, 0, info->dqi_blocks);
if (err)
return err;
prevblk = le32_to_cpu(dh->dqdh_prev_free);
- err = do_check_range(info->dqi_sb, prevblk, info->dqi_blocks);
+ err = do_check_range(info->dqi_sb, prevblk, 0, info->dqi_blocks);
if (err)
return err;
@@ -518,12 +519,10 @@ static int remove_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
goto out_buf;
}
newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
- if (newblk < QT_TREEOFF || newblk >= info->dqi_blocks) {
- quota_error(dquot->dq_sb, "Getting block too big (%u >= %u)",
- newblk, info->dqi_blocks);
- ret = -EUCLEAN;
+ ret = do_check_range(dquot->dq_sb, newblk, QT_TREEOFF,
+ info->dqi_blocks);
+ if (ret)
goto out_buf;
- }
if (depth == info->dqi_qtree_depth - 1) {
ret = free_dqentry(info, dquot, newblk);
@@ -624,12 +623,9 @@ static loff_t find_tree_dqentry(struct qtree_mem_dqinfo *info,
blk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
if (!blk) /* No reference? */
goto out_buf;
- if (blk < QT_TREEOFF || blk >= info->dqi_blocks) {
- quota_error(dquot->dq_sb, "Getting block too big (%u >= %u)",
- blk, info->dqi_blocks);
- ret = -EUCLEAN;
+ ret = do_check_range(dquot->dq_sb, blk, QT_TREEOFF, info->dqi_blocks);
+ if (ret)
goto out_buf;
- }
if (depth < info->dqi_qtree_depth - 1)
ret = find_tree_dqentry(info, dquot, blk, depth+1);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] quota: Add more checking after reading from quota file
2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
@ 2022-09-22 13:04 ` Zhihao Cheng
2022-09-23 11:44 ` Jan Kara
2 siblings, 1 reply; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-22 13:04 UTC (permalink / raw)
To: jack, tytso, brauner
Cc: linux-fsdevel, linux-ext4, linux-kernel, chengzhihao1, yukuai3
It would be better to do more sanity checking (eg. dqdh_entries,
block no.) for the content read from quota file, which can prevent
corrupting the quota file.
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
fs/quota/quota_tree.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index 47711e739ddb..54fe4ad71de5 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -71,12 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
return ret;
}
-static inline int do_check_range(struct super_block *sb, uint val,
- uint min_val, uint max_val)
+static inline int do_check_range(struct super_block *sb, const char *val_name,
+ uint val, uint min_val, uint max_val)
{
if (val < min_val || val >= max_val) {
- quota_error(sb, "Getting block %u out of range %u-%u",
- val, min_val, max_val);
+ quota_error(sb, "Getting %s %u out of range %u-%u",
+ val_name, val, min_val, max_val);
return -EUCLEAN;
}
@@ -90,11 +90,13 @@ static int check_dquot_block_header(struct qtree_mem_dqinfo *info,
uint nextblk, prevblk;
nextblk = le32_to_cpu(dh->dqdh_next_free);
- err = do_check_range(info->dqi_sb, nextblk, 0, info->dqi_blocks);
+ err = do_check_range(info->dqi_sb, "dqdh_next_free", nextblk, 0,
+ info->dqi_blocks);
if (err)
return err;
prevblk = le32_to_cpu(dh->dqdh_prev_free);
- err = do_check_range(info->dqi_sb, prevblk, 0, info->dqi_blocks);
+ err = do_check_range(info->dqi_sb, "dqdh_prev_free", prevblk, 0,
+ info->dqi_blocks);
if (err)
return err;
@@ -268,6 +270,11 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
*err = check_dquot_block_header(info, dh);
if (*err)
goto out_buf;
+ *err = do_check_range(info->dqi_sb, "dqdh_entries",
+ le16_to_cpu(dh->dqdh_entries), 0,
+ qtree_dqstr_in_blk(info));
+ if (*err)
+ goto out_buf;
} else {
blk = get_free_dqblk(info);
if ((int)blk < 0) {
@@ -349,6 +356,10 @@ static int do_insert_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
}
ref = (__le32 *)buf;
newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
+ ret = do_check_range(dquot->dq_sb, "block", newblk, 0,
+ info->dqi_blocks);
+ if (ret)
+ goto out_buf;
if (!newblk)
newson = 1;
if (depth == info->dqi_qtree_depth - 1) {
@@ -461,6 +472,11 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
}
dh = (struct qt_disk_dqdbheader *)buf;
ret = check_dquot_block_header(info, dh);
+ if (ret)
+ goto out_buf;
+ ret = do_check_range(info->dqi_sb, "dqdh_entries",
+ le16_to_cpu(dh->dqdh_entries), 1,
+ qtree_dqstr_in_blk(info) + 1);
if (ret)
goto out_buf;
le16_add_cpu(&dh->dqdh_entries, -1);
@@ -519,7 +535,7 @@ static int remove_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
goto out_buf;
}
newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
- ret = do_check_range(dquot->dq_sb, newblk, QT_TREEOFF,
+ ret = do_check_range(dquot->dq_sb, "block", newblk, QT_TREEOFF,
info->dqi_blocks);
if (ret)
goto out_buf;
@@ -623,7 +639,8 @@ static loff_t find_tree_dqentry(struct qtree_mem_dqinfo *info,
blk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
if (!blk) /* No reference? */
goto out_buf;
- ret = do_check_range(dquot->dq_sb, blk, QT_TREEOFF, info->dqi_blocks);
+ ret = do_check_range(dquot->dq_sb, "block", blk, QT_TREEOFF,
+ info->dqi_blocks);
if (ret)
goto out_buf;
@@ -739,7 +756,13 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
goto out_buf;
}
for (i = __get_index(info, *id, depth); i < epb; i++) {
- if (ref[i] == cpu_to_le32(0)) {
+ uint blk_no = le32_to_cpu(ref[i]);
+
+ ret = do_check_range(info->dqi_sb, "block", blk_no, 0,
+ info->dqi_blocks);
+ if (ret)
+ goto out_buf;
+ if (blk_no == 0) {
*id += level_inc;
continue;
}
@@ -747,7 +770,7 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
ret = 0;
goto out_buf;
}
- ret = find_next_id(info, id, le32_to_cpu(ref[i]), depth + 1);
+ ret = find_next_id(info, id, blk_no, depth + 1);
if (ret != -ENOENT)
break;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] quota: Add more checking after reading from quota file
2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
@ 2022-09-23 11:44 ` Jan Kara
0 siblings, 0 replies; 7+ messages in thread
From: Jan Kara @ 2022-09-23 11:44 UTC (permalink / raw)
To: Zhihao Cheng
Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3
On Thu 22-09-22 21:04:01, Zhihao Cheng wrote:
> It would be better to do more sanity checking (eg. dqdh_entries,
> block no.) for the content read from quota file, which can prevent
> corrupting the quota file.
>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
> fs/quota/quota_tree.c | 43 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> index 47711e739ddb..54fe4ad71de5 100644
> --- a/fs/quota/quota_tree.c
> +++ b/fs/quota/quota_tree.c
> @@ -71,12 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> return ret;
> }
>
> -static inline int do_check_range(struct super_block *sb, uint val,
> - uint min_val, uint max_val)
> +static inline int do_check_range(struct super_block *sb, const char *val_name,
> + uint val, uint min_val, uint max_val)
> {
> if (val < min_val || val >= max_val) {
> - quota_error(sb, "Getting block %u out of range %u-%u",
> - val, min_val, max_val);
> + quota_error(sb, "Getting %s %u out of range %u-%u",
> + val_name, val, min_val, max_val);
> return -EUCLEAN;
> }
As I already wrote in my comments to v1, please create do_check_range()
already with this prototype in patch 1 so that you don't have to update it
(and all the call sites) in each of the patches. It makes review simpler.
> @@ -268,6 +270,11 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
> *err = check_dquot_block_header(info, dh);
> if (*err)
> goto out_buf;
> + *err = do_check_range(info->dqi_sb, "dqdh_entries",
> + le16_to_cpu(dh->dqdh_entries), 0,
> + qtree_dqstr_in_blk(info));
> + if (*err)
> + goto out_buf;
The checking of dqdh_entries belongs into check_dquot_block_header(). That
was the reason why it was created. So that all the checks are together in
one function...
> } else {
> blk = get_free_dqblk(info);
> if ((int)blk < 0) {
> @@ -349,6 +356,10 @@ static int do_insert_tree(struct qtree_mem_dqinfo *info, struct dquot *dquot,
> }
> ref = (__le32 *)buf;
> newblk = le32_to_cpu(ref[get_index(info, dquot->dq_id, depth)]);
> + ret = do_check_range(dquot->dq_sb, "block", newblk, 0,
> + info->dqi_blocks);
> + if (ret)
> + goto out_buf;
> if (!newblk)
> newson = 1;
> if (depth == info->dqi_qtree_depth - 1) {
> @@ -461,6 +472,11 @@ static int free_dqentry(struct qtree_mem_dqinfo *info, struct dquot *dquot,
> }
> dh = (struct qt_disk_dqdbheader *)buf;
> ret = check_dquot_block_header(info, dh);
> + if (ret)
> + goto out_buf;
> + ret = do_check_range(info->dqi_sb, "dqdh_entries",
> + le16_to_cpu(dh->dqdh_entries), 1,
> + qtree_dqstr_in_blk(info) + 1);
Again, the check of dqdh_entries should be in check_dquot_block_header().
> @@ -739,7 +756,13 @@ static int find_next_id(struct qtree_mem_dqinfo *info, qid_t *id,
> goto out_buf;
> }
> for (i = __get_index(info, *id, depth); i < epb; i++) {
> - if (ref[i] == cpu_to_le32(0)) {
> + uint blk_no = le32_to_cpu(ref[i]);
> +
> + ret = do_check_range(info->dqi_sb, "block", blk_no, 0,
> + info->dqi_blocks);
> + if (ret)
> + goto out_buf;
> + if (blk_no == 0) {
> *id += level_inc;
> continue;
> }
I'd leave checking for 0 first here - i.e.:
if (ref[i] == cpu_to_le32(0)) {
*id += level_inc;
continue;
}
and only then do:
blk_no = le32_to_cpu(ref[i]);
ret = do_check_range(...);
There's no point in checking known-good value.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] quota: Replace all block number checking with helper function
2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
@ 2022-09-23 11:48 ` Jan Kara
2022-09-27 1:07 ` Zhihao Cheng
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2022-09-23 11:48 UTC (permalink / raw)
To: Zhihao Cheng
Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3
On Thu 22-09-22 21:04:00, Zhihao Cheng wrote:
> Cleanup all block checking places, replace them with helper function
> do_check_range().
>
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
> fs/quota/quota_tree.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
Thanks for the fix! One comment below:
> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> index f89186b6db1d..47711e739ddb 100644
> --- a/fs/quota/quota_tree.c
> +++ b/fs/quota/quota_tree.c
> @@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> return ret;
> }
>
> -static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
> +static inline int do_check_range(struct super_block *sb, uint val,
> + uint min_val, uint max_val)
> {
> - if (val >= max_val) {
> - quota_error(sb, "Getting block too big (%u >= %u)",
> - val, max_val);
> + if (val < min_val || val >= max_val) {
> + quota_error(sb, "Getting block %u out of range %u-%u",
> + val, min_val, max_val);
> return -EUCLEAN;
> }
It is strange that do_check_range() checks min_val() with strict inequality
and max_val with non-strict one. That's off-by-one problem waiting to
happen when we forget about this detail. Probably make max_val
non-inclusive as well (the parameter max_val suggests the passed value is
the biggest valid one anyway).
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] quota: Replace all block number checking with helper function
2022-09-23 11:48 ` Jan Kara
@ 2022-09-27 1:07 ` Zhihao Cheng
0 siblings, 0 replies; 7+ messages in thread
From: Zhihao Cheng @ 2022-09-27 1:07 UTC (permalink / raw)
To: Jan Kara
Cc: jack, tytso, brauner, linux-fsdevel, linux-ext4, linux-kernel, yukuai3
在 2022/9/23 19:48, Jan Kara 写道:
> On Thu 22-09-22 21:04:00, Zhihao Cheng wrote:
>> Cleanup all block checking places, replace them with helper function
>> do_check_range().
>>
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>> fs/quota/quota_tree.c | 28 ++++++++++++----------------
>> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> Thanks for the fix! One comment below:
>
>> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
>> index f89186b6db1d..47711e739ddb 100644
>> --- a/fs/quota/quota_tree.c
>> +++ b/fs/quota/quota_tree.c
>> @@ -71,11 +71,12 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
>> return ret;
>> }
>>
>> -static inline int do_check_range(struct super_block *sb, uint val, uint max_val)
>> +static inline int do_check_range(struct super_block *sb, uint val,
>> + uint min_val, uint max_val)
>> {
>> - if (val >= max_val) {
>> - quota_error(sb, "Getting block too big (%u >= %u)",
>> - val, max_val);
>> + if (val < min_val || val >= max_val) {
>> + quota_error(sb, "Getting block %u out of range %u-%u",
>> + val, min_val, max_val);
>> return -EUCLEAN;
>> }
>
> It is strange that do_check_range() checks min_val() with strict inequality
> and max_val with non-strict one. That's off-by-one problem waiting to
> happen when we forget about this detail. Probably make max_val
> non-inclusive as well (the parameter max_val suggests the passed value is
> the biggest valid one anyway).
>
> Honza
>
I have sent v3 series, see
https://lore.kernel.org/all/20220923134555.2623931-1-chengzhihao1@huawei.com/T/
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-09-27 1:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 13:03 [PATCH v2 0/3] Check content after reading from quota file Zhihao Cheng
2022-09-22 13:03 ` [PATCH v2 1/3] quota: Check next/prev free block number " Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 2/3] quota: Replace all block number checking with helper function Zhihao Cheng
2022-09-23 11:48 ` Jan Kara
2022-09-27 1:07 ` Zhihao Cheng
2022-09-22 13:04 ` [PATCH v2 3/3] quota: Add more checking after reading from quota file Zhihao Cheng
2022-09-23 11:44 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).