[PATCH 0/3] Check content after reading from quota file

[PATCH 0/3] Check content after reading from quota file

[Zhihao Cheng]

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.

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

[PATCH 1/3] quota: Check next/prev free block number after reading from quota file

[Zhihao Cheng]

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")
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..f2e7f5b869a4 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_free_block(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_free_block(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_free_block(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_free_block(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

[PATCH 2/3] quota: Replace all block number checking with helper function

[Zhihao Cheng]

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 f2e7f5b869a4..efbdef9fbcf3 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_free_block(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

[PATCH 3/3] quota: Add more checking after reading from quota file

[Zhihao Cheng]

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 efbdef9fbcf3..bca71640d0ae 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_free_block(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_free_block(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_free_block(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

Re: [PATCH 0/3] Check content after reading from quota file

[Zhihao Cheng]

在 2022/8/20 19:05, Zhihao Cheng 写道:

friendly ping.

> 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.
>
> 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(-)
>

Re: [PATCH 0/3] Check content after reading from quota file

[Jan Kara]

On Tue 13-09-22 09:16:11, Zhihao Cheng wrote:
> 在 2022/8/20 19:05, Zhihao Cheng 写道:
> 
> friendly ping.

I'm sorry for the delay. Somehow our corporate spam filter decided to
discard your patches. I'll have a look into your fixes shortly.

								Honza

> > 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.
> > 
> > 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(-)
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] quota: Check next/prev free block number after reading from quota file

[Jan Kara]

On Sat 20-08-22 19:05:12, Zhihao Cheng wrote:
> 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")

It's better to just have:

CC: stable@vger.kernel.org

here. Fixes tag pointing to kernel release is not very useful.

> --- 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;
> +}

I'd already provide min_val and the string for the message here as well (as
you do in patch 2). It is less churn in the next patch and free blocks
checking actually needs that as well. See below.

> +
> +static int check_free_block(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;

The free block should actually be > QT_TREEOFF so I'd add the check to
do_check_range().

Also rather than having check_free_block(), I'd provide a helper function
like check_dquot_block_header() which will check only free blocks pointers
now and in later patches you can add other checks there.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] quota: Check next/prev free block number after reading from quota file

[Zhihao Cheng]

在 2022/9/21 21:37, Jan Kara 写道:
Hi Jan,
> On Sat 20-08-22 19:05:12, Zhihao Cheng wrote:
>> Following process:
[...]
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216372
>> Fixes: 1da177e4c3f4152 ("Linux-2.6.12-rc2")
> 
> It's better to just have:
> 
> CC: stable@vger.kernel.org
> 
> here. Fixes tag pointing to kernel release is not very useful.
Will add in v2.
> 
>> --- 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;
>> +}
> 
> I'd already provide min_val and the string for the message here as well (as
> you do in patch 2). It is less churn in the next patch and free blocks
> checking actually needs that as well. See below.
> 
>> +
>> +static int check_free_block(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;
> 
> The free block should actually be > QT_TREEOFF so I'd add the check to
> do_check_range().

'dh->dqdh_next_free' may be updated when quota entry removed, 
'dh->dqdh_next_free' can be used for next new quota entris.
Before sending v2, I found 'dh->dqdh_next_free' and 'dh->dqdh_prev_free' 
can easily be zero in newly allocated blocks when continually creating 
files onwed by different users:
find_free_dqentry
   get_free_dqblk
     write_blk(info, info->dqi_blocks, buf)  // zero'd qt_disk_dqdbheader
     blk = info->dqi_blocks++   // allocate new one block
   info->dqi_free_entry = blk   // will be used for new quota entries

find_free_dqentry
   if (info->dqi_free_entry)
     blk = info->dqi_free_entry
     read_blk(info, blk, buf)   // dh->dqdh_next_free = 
dh->dqdh_prev_free = 0

I think it's normal when 'dh->dqdh_next_free' or 'dh->dqdh_prev_free' 
equals to 0.
> 
> Also rather than having check_free_block(), I'd provide a helper function
> like check_dquot_block_header() which will check only free blocks pointers
> now and in later patches you can add other checks there.
OK, will be updated in v2.
> 
> 								Honza
> 

Re: [PATCH 1/3] quota: Check next/prev free block number after reading from quota file

[Jan Kara]

On Thu 22-09-22 16:13:59, Zhihao Cheng wrote:
> 在 2022/9/21 21:37, Jan Kara 写道:
> > > --- 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;
> > > +}
> > 
> > I'd already provide min_val and the string for the message here as well (as
> > you do in patch 2). It is less churn in the next patch and free blocks
> > checking actually needs that as well. See below.
> > 
> > > +
> > > +static int check_free_block(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;
> > 
> > The free block should actually be > QT_TREEOFF so I'd add the check to
> > do_check_range().
> 
> 'dh->dqdh_next_free' may be updated when quota entry removed,
> 'dh->dqdh_next_free' can be used for next new quota entris.
> Before sending v2, I found 'dh->dqdh_next_free' and 'dh->dqdh_prev_free' can
> easily be zero in newly allocated blocks when continually creating files
> onwed by different users:
> find_free_dqentry
>   get_free_dqblk
>     write_blk(info, info->dqi_blocks, buf)  // zero'd qt_disk_dqdbheader
>     blk = info->dqi_blocks++   // allocate new one block
>   info->dqi_free_entry = blk   // will be used for new quota entries
> 
> find_free_dqentry
>   if (info->dqi_free_entry)
>     blk = info->dqi_free_entry
>     read_blk(info, blk, buf)   // dh->dqdh_next_free = dh->dqdh_prev_free =
> 0
> 
> I think it's normal when 'dh->dqdh_next_free' or 'dh->dqdh_prev_free' equals
> to 0.

Good point! 0 means "not present". So any block number (either in free list
or pointed from the quota tree) should be either 0 or > QT_TREEOFF.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/3] quota: Check next/prev free block number after reading from quota file

[Zhihao Cheng]

在 2022/9/22 19:39, Jan Kara 写道:
> On Thu 22-09-22 16:13:59, Zhihao Cheng wrote:
[...]
>>>
>>> The free block should actually be > QT_TREEOFF so I'd add the check to
>>> do_check_range().
>>
>> 'dh->dqdh_next_free' may be updated when quota entry removed,
>> 'dh->dqdh_next_free' can be used for next new quota entris.
>> Before sending v2, I found 'dh->dqdh_next_free' and 'dh->dqdh_prev_free' can
>> easily be zero in newly allocated blocks when continually creating files
>> onwed by different users:
>> find_free_dqentry
>>    get_free_dqblk
>>      write_blk(info, info->dqi_blocks, buf)  // zero'd qt_disk_dqdbheader
>>      blk = info->dqi_blocks++   // allocate new one block
>>    info->dqi_free_entry = blk   // will be used for new quota entries
>>
>> find_free_dqentry
>>    if (info->dqi_free_entry)
>>      blk = info->dqi_free_entry
>>      read_blk(info, blk, buf)   // dh->dqdh_next_free = dh->dqdh_prev_free =
>> 0
>>
>> I think it's normal when 'dh->dqdh_next_free' or 'dh->dqdh_prev_free' equals
>> to 0.
> 
> Good point! 0 means "not present". So any block number (either in free list
> or pointed from the quota tree) should be either 0 or > QT_TREEOFF.
> 
> 								Honza
> 

In case my emails being filtered agagin, this is notification of v2:
https://lore.kernel.org/linux-ext4/20220922130401.1792256-1-chengzhihao1@huawei.com/T/#m9676d64e8f7cdd7b7decdd0d6b725ec658110b3e