[f2fs-dev] [PATCH] fsck: prevent buffer overrun in quota code

[f2fs-dev] [PATCH] fsck: prevent buffer overrun in quota code

[Jaegeuk Kim]

From: Robin Hsu <robinhsu@google.com>

A maliciously corrupted file systems can trigger buffer overruns in
the quota code used by fsck.

To fix it, quota file sizes are checked against real allocated
block index tables (inode, direct nodes, indirect nodes, double
indirect nodes).  If the size mismatches, the quota file is considered
corrupted and will be regenerated.

Signed-off-by: Robin Hsu <robinhsu@google.com>
---
 fsck/fsck.c         | 21 +++++++++++++++++-
 fsck/fsck.h         |  2 ++
 fsck/mkquota.c      |  1 +
 fsck/quotaio.c      | 10 ++++++++-
 fsck/quotaio.h      | 13 ++++++++++-
 fsck/quotaio_tree.c | 14 +++++++-----
 fsck/quotaio_v2.c   | 53 +++++++++++++++++++++++++++++++++++++++++----
 fsck/segment.c      | 21 ++++++++++++++++++
 8 files changed, 122 insertions(+), 13 deletions(-)

diff --git a/fsck/fsck.c b/fsck/fsck.c
index c249dfa..9a537af 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -792,6 +792,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 
 	if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) {
 		unsigned int inline_size = MAX_INLINE_DATA(node_blk);
+		if (cur_qtype != -1)
+			qf_szchk_type[cur_qtype] = QF_SZCHK_INLINE;
 		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs]);
 
 		if (blkaddr != 0) {
@@ -860,6 +862,15 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 	}
 
 	/* check data blocks in inode */
+	if (cur_qtype != -1) {
+		qf_szchk_type[cur_qtype] = QF_SZCHK_REGFILE;
+		qf_maxsize[cur_qtype] = (ADDRS_PER_INODE(&node_blk->i) +
+				2 * ADDRS_PER_BLOCK(&node_blk->i) +
+				2 * ADDRS_PER_BLOCK(&node_blk->i) *
+				NIDS_PER_BLOCK +
+				(u64) ADDRS_PER_BLOCK(&node_blk->i) *
+				NIDS_PER_BLOCK * NIDS_PER_BLOCK) * F2FS_BLKSIZE;
+	}
 	for (idx = 0; idx < ADDRS_PER_INODE(&node_blk->i);
 						idx++, child.pgofs++) {
 		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs + idx]);
@@ -884,6 +895,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 					file_is_encrypt(&node_blk->i));
 			if (!ret) {
 				*blk_cnt = *blk_cnt + 1;
+				if (cur_qtype != -1 && blkaddr != NEW_ADDR)
+					qf_size_chk[cur_qtype] = child.pgofs;
 			} else if (c.fix_on) {
 				node_blk->i.i_addr[ofs + idx] = 0;
 				need_fix = 1;
@@ -1126,6 +1139,8 @@ int fsck_chk_dnode_blk(struct f2fs_sb_info *sbi, struct f2fs_inode *inode,
 			file_is_encrypt(inode));
 		if (!ret) {
 			*blk_cnt = *blk_cnt + 1;
+			if (cur_qtype != -1 && blkaddr != NEW_ADDR)
+				qf_size_chk[cur_qtype] = child->pgofs;
 		} else if (c.fix_on) {
 			node_blk->dn.addr[idx] = 0;
 			need_fix = 1;
@@ -1794,6 +1809,7 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
 	u32 blk_cnt = 0;
 
 	for (qtype = 0; qtype < F2FS_MAX_QUOTAS; qtype++) {
+		cur_qtype = qtype;
 		if (sb->qf_ino[qtype] == 0)
 			continue;
 		nid_t ino = QUOTA_INO(sb, qtype);
@@ -1811,10 +1827,13 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
 		}
 		ret = fsck_chk_node_blk(sbi, NULL, ino,
 				F2FS_FT_REG_FILE, TYPE_INODE, &blk_cnt, NULL);
-		if (ret)
+		if (ret) {
 			ASSERT_MSG("wrong quota inode, qtype [%d] ino [0x%x]",
 								qtype, ino);
+			qf_szchk_type[qtype] = QF_SZCHK_ERR;
+		}
 	}
+	cur_qtype = -1;
 	return ret;
 }
 
diff --git a/fsck/fsck.h b/fsck/fsck.h
index 2de6f62..bc6a435 100644
--- a/fsck/fsck.h
+++ b/fsck/fsck.h
@@ -266,6 +266,8 @@ block_t new_node_block(struct f2fs_sb_info *,
 					struct dnode_of_data *, unsigned int);
 
 /* segment.c */
+struct quota_file;
+u64 f2fs_quota_size(struct quota_file *);
 u64 f2fs_read(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
 u64 f2fs_write(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
 void f2fs_filesize_update(struct f2fs_sb_info *, nid_t, u64);
diff --git a/fsck/mkquota.c b/fsck/mkquota.c
index 84f9d3d..c419a0f 100644
--- a/fsck/mkquota.c
+++ b/fsck/mkquota.c
@@ -378,6 +378,7 @@ errcode_t quota_compare_and_update(struct f2fs_sb_info *sbi,
 	err = quota_file_open(sbi, &qh, qtype, 0);
 	if (err) {
 		log_debug("Open quota file failed");
+		*usage_inconsistent = 1;
 		goto out;
 	}
 
diff --git a/fsck/quotaio.c b/fsck/quotaio.c
index cc517bd..759c9e1 100644
--- a/fsck/quotaio.c
+++ b/fsck/quotaio.c
@@ -33,6 +33,14 @@ struct disk_dqheader {
 	__le32 dqh_version;
 } __attribute__ ((packed));
 
+int cur_qtype = -1;
+u32 qf_size_chk[MAXQUOTAS] = {0, 0, 0};
+enum qf_szchk_type_t qf_szchk_type[MAXQUOTAS] =
+{
+	QF_SZCHK_NONE, QF_SZCHK_NONE, QF_SZCHK_NONE
+};
+u64 qf_maxsize[MAXQUOTAS];
+
 /**
  * Convert type of quota to written representation
  */
@@ -140,7 +148,7 @@ errcode_t quota_file_open(struct f2fs_sb_info *sbi, struct quota_handle *h,
 		goto errout;
 	}
 
-	if (h->qh_ops->init_io && (h->qh_ops->init_io(h) < 0)) {
+	if (h->qh_ops->init_io && (h->qh_ops->init_io(h, qtype) < 0)) {
 		log_err("qh_ops->init_io failed");
 		err = EIO;
 		goto errout;
diff --git a/fsck/quotaio.h b/fsck/quotaio.h
index 8087309..4a366ce 100644
--- a/fsck/quotaio.h
+++ b/fsck/quotaio.h
@@ -46,6 +46,17 @@ enum quota_type {
 #error "cannot have more than 32 quota types to fit in qtype_bits"
 #endif
 
+enum qf_szchk_type_t {
+	QF_SZCHK_NONE,
+	QF_SZCHK_ERR,
+	QF_SZCHK_INLINE,
+	QF_SZCHK_REGFILE,
+};
+
+extern int cur_qtype;
+extern u32 qf_size_chk[];
+extern enum qf_szchk_type_t qf_szchk_type[];
+extern u64 qf_maxsize[];
 
 #define QUOTA_USR_BIT (1 << USRQUOTA)
 #define QUOTA_GRP_BIT (1 << GRPQUOTA)
@@ -154,7 +165,7 @@ struct quotafile_ops {
 	/* Check whether quotafile is in our format */
 	int (*check_file) (struct quota_handle *h, int type);
 	/* Open quotafile */
-	int (*init_io) (struct quota_handle *h);
+	int (*init_io) (struct quota_handle *h, enum quota_type qtype);
 	/* Create new quotafile */
 	int (*new_io) (struct quota_handle *h);
 	/* Write all changes and close quotafile */
diff --git a/fsck/quotaio_tree.c b/fsck/quotaio_tree.c
index de25a60..c203400 100644
--- a/fsck/quotaio_tree.c
+++ b/fsck/quotaio_tree.c
@@ -568,7 +568,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
 	int entries, i;
 
 	if (!buf)
-		return 0;
+		return -1;
 
 	set_bit(bitmap, blk);
 	read_blk(dquot->dq_h, blk, buf);
@@ -593,9 +593,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
 static int check_reference(struct quota_handle *h, unsigned int blk)
 {
 	if (blk >= h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks) {
-		log_err("Illegal reference (%u >= %u) in %s quota file. "
-			"Quota file is probably corrupted.\n"
-			"Please run fsck (8) to fix it.",
+		log_err("Illegal reference (%u >= %u) in %s quota file",
 			blk,
 			h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks,
 			quota_type2name(h->qh_type));
@@ -627,9 +625,13 @@ static int report_tree(struct dquot *dquot, unsigned int blk, int depth,
 			break;
 
 		if (depth == QT_TREEDEPTH - 1) {
-			if (!get_bit(bitmap, blk))
-				*entries += report_block(dquot, blk, bitmap,
+			if (!get_bit(bitmap, blk)) {
+				int num_entry = report_block(dquot, blk, bitmap,
 							process_dquot, data);
+				if (num_entry < 0)
+					break;
+				*entries += num_entry;
+			}
 		} else {
 			if (report_tree(dquot, blk, depth + 1, bitmap, entries,
 						process_dquot, data))
diff --git a/fsck/quotaio_v2.c b/fsck/quotaio_v2.c
index 1404332..0f8672b 100644
--- a/fsck/quotaio_v2.c
+++ b/fsck/quotaio_v2.c
@@ -20,7 +20,7 @@
 #include "quotaio_tree.h"
 
 static int v2_check_file(struct quota_handle *h, int type);
-static int v2_init_io(struct quota_handle *h);
+static int v2_init_io(struct quota_handle *h, enum quota_type qtype);
 static int v2_new_io(struct quota_handle *h);
 static int v2_write_info(struct quota_handle *h);
 static struct dquot *v2_read_dquot(struct quota_handle *h, qid_t id);
@@ -170,19 +170,64 @@ static int v2_check_file(struct quota_handle *h, int type)
 /*
  * Open quotafile
  */
-static int v2_init_io(struct quota_handle *h)
+static int v2_init_io(struct quota_handle *h, enum quota_type qtype)
 {
 	struct v2_disk_dqinfo ddqinfo;
+	struct v2_mem_dqinfo *info;
+	u64 filesize;
+	struct quota_file *qf = &h->qh_qf;
 
 	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_entry_size =
 		sizeof(struct v2r1_disk_dqblk);
 	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_ops = &v2r1_fmt_ops;
 
 	/* Read information about quotafile */
-	if (h->read(&h->qh_qf, V2_DQINFOOFF, &ddqinfo,
-			 sizeof(ddqinfo)) != sizeof(ddqinfo))
+	if (h->read(qf, V2_DQINFOOFF, &ddqinfo,
+			sizeof(ddqinfo)) != sizeof(ddqinfo))
 		return -1;
 	v2_disk2memdqinfo(&h->qh_info, &ddqinfo);
+
+	/* Check to make sure quota file info is sane */
+	info = &h->qh_info.u.v2_mdqi;
+	filesize = qf->filesize = f2fs_quota_size(qf);
+	u32 qf_szchk = qf_size_chk[qtype];
+	if (qf_szchk_type[qtype] == QF_SZCHK_REGFILE &&
+			((filesize + F2FS_BLKSIZE - 1) >> F2FS_BLKSIZE_BITS <
+			qf_szchk + 1 || filesize > qf_maxsize[qtype])) {
+		/*
+		 * reqular: qf_szchk is now the last block index,
+		 * including the hole's index
+		 */
+		log_err("Quota inode %u corrupted: file size %" PRIu64
+			" does not match page offset %" PRIu32,
+			h->qh_qf.ino,
+			filesize,
+			qf_szchk);
+		filesize = (qf_szchk + 1) << F2FS_BLKSIZE_BITS;
+		f2fs_filesize_update(qf->sbi, qf->ino, filesize);
+	}
+
+	if ((info->dqi_qtree.dqi_blocks >
+			(filesize + QT_BLKSIZE - 1) >> QT_BLKSIZE_BITS)) {
+		log_err("Quota inode %u corrupted: file size %" PRId64 "; "
+				"dqi_blocks %u", h->qh_qf.ino,
+				filesize, info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
+	if (info->dqi_qtree.dqi_free_blk >= info->dqi_qtree.dqi_blocks) {
+		log_err("Quota inode %u corrupted: free_blk %u;"
+				" dqi_blocks %u",
+				h->qh_qf.ino, info->dqi_qtree.dqi_free_blk,
+				info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
+	if (info->dqi_qtree.dqi_free_entry >= info->dqi_qtree.dqi_blocks) {
+		log_err("Quota inode %u corrupted: free_entry %u; "
+				"dqi_blocks %u", h->qh_qf.ino,
+				info->dqi_qtree.dqi_free_entry,
+				info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
 	return 0;
 }
 
diff --git a/fsck/segment.c b/fsck/segment.c
index b7cf245..a5dc556 100644
--- a/fsck/segment.c
+++ b/fsck/segment.c
@@ -15,6 +15,7 @@
  */
 #include "fsck.h"
 #include "node.h"
+#include "quotaio.h"
 
 int reserve_new_block(struct f2fs_sb_info *sbi, block_t *to,
 			struct f2fs_summary *sum, int type, bool is_inode)
@@ -124,6 +125,26 @@ int new_data_block(struct f2fs_sb_info *sbi, void *block,
 	return 0;
 }
 
+u64 f2fs_quota_size(struct quota_file *qf)
+{
+	struct node_info ni;
+	struct f2fs_node *inode;
+	u64 filesize;
+
+	inode = (struct f2fs_node *) calloc(BLOCK_SZ, 1);
+	ASSERT(inode);
+
+	/* Read inode */
+	get_node_info(qf->sbi, qf->ino, &ni);
+	ASSERT(dev_read_block(inode, ni.blk_addr) >= 0);
+	ASSERT(!S_ISDIR(le16_to_cpu(inode->i.i_mode)));
+	ASSERT(!S_ISLNK(le16_to_cpu(inode->i.i_mode)));
+
+	filesize = le64_to_cpu(inode->i.i_size);
+	free(inode);
+	return filesize;
+}
+
 u64 f2fs_read(struct f2fs_sb_info *sbi, nid_t ino, u8 *buffer,
 					u64 count, pgoff_t offset)
 {
-- 
2.27.0.290.gba653c62da-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] fsck: prevent buffer overrun in quota code

[Chao Yu]

On 2020/6/17 7:42, Jaegeuk Kim wrote:
> From: Robin Hsu <robinhsu@google.com>
> 
> A maliciously corrupted file systems can trigger buffer overruns in
> the quota code used by fsck.
> 
> To fix it, quota file sizes are checked against real allocated
> block index tables (inode, direct nodes, indirect nodes, double
> indirect nodes).  If the size mismatches, the quota file is considered
> corrupted and will be regenerated.
> 
> Signed-off-by: Robin Hsu <robinhsu@google.com>
> ---
>  fsck/fsck.c         | 21 +++++++++++++++++-
>  fsck/fsck.h         |  2 ++
>  fsck/mkquota.c      |  1 +
>  fsck/quotaio.c      | 10 ++++++++-
>  fsck/quotaio.h      | 13 ++++++++++-
>  fsck/quotaio_tree.c | 14 +++++++-----
>  fsck/quotaio_v2.c   | 53 +++++++++++++++++++++++++++++++++++++++++----
>  fsck/segment.c      | 21 ++++++++++++++++++
>  8 files changed, 122 insertions(+), 13 deletions(-)
> 
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index c249dfa..9a537af 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -792,6 +792,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
>  
>  	if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) {
>  		unsigned int inline_size = MAX_INLINE_DATA(node_blk);
> +		if (cur_qtype != -1)
> +			qf_szchk_type[cur_qtype] = QF_SZCHK_INLINE;
>  		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs]);
>  
>  		if (blkaddr != 0) {
> @@ -860,6 +862,15 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
>  	}
>  
>  	/* check data blocks in inode */
> +	if (cur_qtype != -1) {
> +		qf_szchk_type[cur_qtype] = QF_SZCHK_REGFILE;
> +		qf_maxsize[cur_qtype] = (ADDRS_PER_INODE(&node_blk->i) +
> +				2 * ADDRS_PER_BLOCK(&node_blk->i) +
> +				2 * ADDRS_PER_BLOCK(&node_blk->i) *
> +				NIDS_PER_BLOCK +
> +				(u64) ADDRS_PER_BLOCK(&node_blk->i) *
> +				NIDS_PER_BLOCK * NIDS_PER_BLOCK) * F2FS_BLKSIZE;
> +	}
>  	for (idx = 0; idx < ADDRS_PER_INODE(&node_blk->i);
>  						idx++, child.pgofs++) {
>  		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs + idx]);
> @@ -884,6 +895,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
>  					file_is_encrypt(&node_blk->i));
>  			if (!ret) {
>  				*blk_cnt = *blk_cnt + 1;
> +				if (cur_qtype != -1 && blkaddr != NEW_ADDR)
> +					qf_size_chk[cur_qtype] = child.pgofs;
>  			} else if (c.fix_on) {
>  				node_blk->i.i_addr[ofs + idx] = 0;
>  				need_fix = 1;
> @@ -1126,6 +1139,8 @@ int fsck_chk_dnode_blk(struct f2fs_sb_info *sbi, struct f2fs_inode *inode,
>  			file_is_encrypt(inode));
>  		if (!ret) {
>  			*blk_cnt = *blk_cnt + 1;
> +			if (cur_qtype != -1 && blkaddr != NEW_ADDR)
> +				qf_size_chk[cur_qtype] = child->pgofs;
>  		} else if (c.fix_on) {
>  			node_blk->dn.addr[idx] = 0;
>  			need_fix = 1;
> @@ -1794,6 +1809,7 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
>  	u32 blk_cnt = 0;
>  
>  	for (qtype = 0; qtype < F2FS_MAX_QUOTAS; qtype++) {
> +		cur_qtype = qtype;
>  		if (sb->qf_ino[qtype] == 0)
>  			continue;
>  		nid_t ino = QUOTA_INO(sb, qtype);
> @@ -1811,10 +1827,13 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
>  		}
>  		ret = fsck_chk_node_blk(sbi, NULL, ino,
>  				F2FS_FT_REG_FILE, TYPE_INODE, &blk_cnt, NULL);
> -		if (ret)
> +		if (ret) {
>  			ASSERT_MSG("wrong quota inode, qtype [%d] ino [0x%x]",
>  								qtype, ino);
> +			qf_szchk_type[qtype] = QF_SZCHK_ERR;
> +		}
>  	}
> +	cur_qtype = -1;
>  	return ret;
>  }
>  
> diff --git a/fsck/fsck.h b/fsck/fsck.h
> index 2de6f62..bc6a435 100644
> --- a/fsck/fsck.h
> +++ b/fsck/fsck.h
> @@ -266,6 +266,8 @@ block_t new_node_block(struct f2fs_sb_info *,
>  					struct dnode_of_data *, unsigned int);
>  
>  /* segment.c */
> +struct quota_file;
> +u64 f2fs_quota_size(struct quota_file *);
>  u64 f2fs_read(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
>  u64 f2fs_write(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
>  void f2fs_filesize_update(struct f2fs_sb_info *, nid_t, u64);
> diff --git a/fsck/mkquota.c b/fsck/mkquota.c
> index 84f9d3d..c419a0f 100644
> --- a/fsck/mkquota.c
> +++ b/fsck/mkquota.c
> @@ -378,6 +378,7 @@ errcode_t quota_compare_and_update(struct f2fs_sb_info *sbi,
>  	err = quota_file_open(sbi, &qh, qtype, 0);
>  	if (err) {
>  		log_debug("Open quota file failed");
> +		*usage_inconsistent = 1;
>  		goto out;
>  	}
>  
> diff --git a/fsck/quotaio.c b/fsck/quotaio.c
> index cc517bd..759c9e1 100644
> --- a/fsck/quotaio.c
> +++ b/fsck/quotaio.c
> @@ -33,6 +33,14 @@ struct disk_dqheader {
>  	__le32 dqh_version;
>  } __attribute__ ((packed));
>  
> +int cur_qtype = -1;
> +u32 qf_size_chk[MAXQUOTAS] = {0, 0, 0};
> +enum qf_szchk_type_t qf_szchk_type[MAXQUOTAS] =
> +{
> +	QF_SZCHK_NONE, QF_SZCHK_NONE, QF_SZCHK_NONE
> +};
> +u64 qf_maxsize[MAXQUOTAS];
> +
>  /**
>   * Convert type of quota to written representation
>   */
> @@ -140,7 +148,7 @@ errcode_t quota_file_open(struct f2fs_sb_info *sbi, struct quota_handle *h,
>  		goto errout;
>  	}
>  
> -	if (h->qh_ops->init_io && (h->qh_ops->init_io(h) < 0)) {
> +	if (h->qh_ops->init_io && (h->qh_ops->init_io(h, qtype) < 0)) {
>  		log_err("qh_ops->init_io failed");
>  		err = EIO;
>  		goto errout;
> diff --git a/fsck/quotaio.h b/fsck/quotaio.h
> index 8087309..4a366ce 100644
> --- a/fsck/quotaio.h
> +++ b/fsck/quotaio.h
> @@ -46,6 +46,17 @@ enum quota_type {
>  #error "cannot have more than 32 quota types to fit in qtype_bits"
>  #endif
>  
> +enum qf_szchk_type_t {
> +	QF_SZCHK_NONE,
> +	QF_SZCHK_ERR,
> +	QF_SZCHK_INLINE,
> +	QF_SZCHK_REGFILE,
> +};
> +
> +extern int cur_qtype;
> +extern u32 qf_size_chk[];
> +extern enum qf_szchk_type_t qf_szchk_type[];
> +extern u64 qf_maxsize[];
>  
>  #define QUOTA_USR_BIT (1 << USRQUOTA)
>  #define QUOTA_GRP_BIT (1 << GRPQUOTA)
> @@ -154,7 +165,7 @@ struct quotafile_ops {
>  	/* Check whether quotafile is in our format */
>  	int (*check_file) (struct quota_handle *h, int type);
>  	/* Open quotafile */
> -	int (*init_io) (struct quota_handle *h);
> +	int (*init_io) (struct quota_handle *h, enum quota_type qtype);
>  	/* Create new quotafile */
>  	int (*new_io) (struct quota_handle *h);
>  	/* Write all changes and close quotafile */
> diff --git a/fsck/quotaio_tree.c b/fsck/quotaio_tree.c
> index de25a60..c203400 100644
> --- a/fsck/quotaio_tree.c
> +++ b/fsck/quotaio_tree.c
> @@ -568,7 +568,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
>  	int entries, i;
>  
>  	if (!buf)
> -		return 0;
> +		return -1;
>  
>  	set_bit(bitmap, blk);
>  	read_blk(dquot->dq_h, blk, buf);
> @@ -593,9 +593,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
>  static int check_reference(struct quota_handle *h, unsigned int blk)
>  {
>  	if (blk >= h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks) {
> -		log_err("Illegal reference (%u >= %u) in %s quota file. "
> -			"Quota file is probably corrupted.\n"
> -			"Please run fsck (8) to fix it.",
> +		log_err("Illegal reference (%u >= %u) in %s quota file",
>  			blk,
>  			h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks,
>  			quota_type2name(h->qh_type));
> @@ -627,9 +625,13 @@ static int report_tree(struct dquot *dquot, unsigned int blk, int depth,
>  			break;
>  
>  		if (depth == QT_TREEDEPTH - 1) {
> -			if (!get_bit(bitmap, blk))
> -				*entries += report_block(dquot, blk, bitmap,
> +			if (!get_bit(bitmap, blk)) {
> +				int num_entry = report_block(dquot, blk, bitmap,
>  							process_dquot, data);
> +				if (num_entry < 0)
> +					break;
> +				*entries += num_entry;
> +			}
>  		} else {
>  			if (report_tree(dquot, blk, depth + 1, bitmap, entries,
>  						process_dquot, data))
> diff --git a/fsck/quotaio_v2.c b/fsck/quotaio_v2.c
> index 1404332..0f8672b 100644
> --- a/fsck/quotaio_v2.c
> +++ b/fsck/quotaio_v2.c
> @@ -20,7 +20,7 @@
>  #include "quotaio_tree.h"
>  
>  static int v2_check_file(struct quota_handle *h, int type);
> -static int v2_init_io(struct quota_handle *h);
> +static int v2_init_io(struct quota_handle *h, enum quota_type qtype);
>  static int v2_new_io(struct quota_handle *h);
>  static int v2_write_info(struct quota_handle *h);
>  static struct dquot *v2_read_dquot(struct quota_handle *h, qid_t id);
> @@ -170,19 +170,64 @@ static int v2_check_file(struct quota_handle *h, int type)
>  /*
>   * Open quotafile
>   */
> -static int v2_init_io(struct quota_handle *h)
> +static int v2_init_io(struct quota_handle *h, enum quota_type qtype)
>  {
>  	struct v2_disk_dqinfo ddqinfo;
> +	struct v2_mem_dqinfo *info;
> +	u64 filesize;
> +	struct quota_file *qf = &h->qh_qf;
>  
>  	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_entry_size =
>  		sizeof(struct v2r1_disk_dqblk);
>  	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_ops = &v2r1_fmt_ops;
>  
>  	/* Read information about quotafile */
> -	if (h->read(&h->qh_qf, V2_DQINFOOFF, &ddqinfo,
> -			 sizeof(ddqinfo)) != sizeof(ddqinfo))
> +	if (h->read(qf, V2_DQINFOOFF, &ddqinfo,
> +			sizeof(ddqinfo)) != sizeof(ddqinfo))
>  		return -1;
>  	v2_disk2memdqinfo(&h->qh_info, &ddqinfo);
> +
> +	/* Check to make sure quota file info is sane */
> +	info = &h->qh_info.u.v2_mdqi;
> +	filesize = qf->filesize = f2fs_quota_size(qf);
> +	u32 qf_szchk = qf_size_chk[qtype];
> +	if (qf_szchk_type[qtype] == QF_SZCHK_REGFILE &&
> +			((filesize + F2FS_BLKSIZE - 1) >> F2FS_BLKSIZE_BITS <
> +			qf_szchk + 1 || filesize > qf_maxsize[qtype])) {
> +		/*
> +		 * reqular: qf_szchk is now the last block index,
> +		 * including the hole's index

If so, I guess qf_last_blkofs will be more proper for the variable name.

> +		 */
> +		log_err("Quota inode %u corrupted: file size %" PRIu64
> +			" does not match page offset %" PRIu32,
> +			h->qh_qf.ino,
> +			filesize,
> +			qf_szchk);
> +		filesize = (qf_szchk + 1) << F2FS_BLKSIZE_BITS;
> +		f2fs_filesize_update(qf->sbi, qf->ino, filesize);
> +	}
> +
> +	if ((info->dqi_qtree.dqi_blocks >
> +			(filesize + QT_BLKSIZE - 1) >> QT_BLKSIZE_BITS)) {
> +		log_err("Quota inode %u corrupted: file size %" PRId64 "; "
> +				"dqi_blocks %u", h->qh_qf.ino,
> +				filesize, info->dqi_qtree.dqi_blocks);
> +		return -1;
> +	}
> +	if (info->dqi_qtree.dqi_free_blk >= info->dqi_qtree.dqi_blocks) {
> +		log_err("Quota inode %u corrupted: free_blk %u;"
> +				" dqi_blocks %u",
> +				h->qh_qf.ino, info->dqi_qtree.dqi_free_blk,
> +				info->dqi_qtree.dqi_blocks);
> +		return -1;
> +	}
> +	if (info->dqi_qtree.dqi_free_entry >= info->dqi_qtree.dqi_blocks) {
> +		log_err("Quota inode %u corrupted: free_entry %u; "
> +				"dqi_blocks %u", h->qh_qf.ino,
> +				info->dqi_qtree.dqi_free_entry,
> +				info->dqi_qtree.dqi_blocks);
> +		return -1;
> +	}
>  	return 0;
>  }
>  
> diff --git a/fsck/segment.c b/fsck/segment.c
> index b7cf245..a5dc556 100644
> --- a/fsck/segment.c
> +++ b/fsck/segment.c
> @@ -15,6 +15,7 @@
>   */
>  #include "fsck.h"
>  #include "node.h"
> +#include "quotaio.h"
>  
>  int reserve_new_block(struct f2fs_sb_info *sbi, block_t *to,
>  			struct f2fs_summary *sum, int type, bool is_inode)
> @@ -124,6 +125,26 @@ int new_data_block(struct f2fs_sb_info *sbi, void *block,
>  	return 0;
>  }
>  
> +u64 f2fs_quota_size(struct quota_file *qf)
> +{
> +	struct node_info ni;
> +	struct f2fs_node *inode;
> +	u64 filesize;
> +
> +	inode = (struct f2fs_node *) calloc(BLOCK_SZ, 1);
> +	ASSERT(inode);
> +
> +	/* Read inode */
> +	get_node_info(qf->sbi, qf->ino, &ni);
> +	ASSERT(dev_read_block(inode, ni.blk_addr) >= 0);
> +	ASSERT(!S_ISDIR(le16_to_cpu(inode->i.i_mode)));
> +	ASSERT(!S_ISLNK(le16_to_cpu(inode->i.i_mode)));

Why not ASSERT(S_ISREG())? or we can introduce another function to repair
such fuzzed quota inode?

Thanks,

> +
> +	filesize = le64_to_cpu(inode->i.i_size);
> +	free(inode);
> +	return filesize;
> +}
> +
>  u64 f2fs_read(struct f2fs_sb_info *sbi, nid_t ino, u8 *buffer,
>  					u64 count, pgoff_t offset)
>  {
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2] fsck: prevent buffer overrun in quota code

[Jaegeuk Kim]

A maliciously corrupted file systems can trigger buffer overruns in
the quota code used by fsck.

To fix it, quota file sizes are checked against real allocated
block index tables (inode, direct nodes, indirect nodes, double
indirect nodes).  If the size mismatches, the quota file is considered
corrupted and will be regenerated.

Signed-off-by: Robin Hsu <robinhsu@google.com>
---
Change log from v1:
 - use S_ISREG()
 - qf_size_chk -> qf_last_blkofs, qf_szchk -> last_blkofs

 fsck/fsck.c         | 21 +++++++++++++++++-
 fsck/fsck.h         |  2 ++
 fsck/mkquota.c      |  1 +
 fsck/quotaio.c      | 10 ++++++++-
 fsck/quotaio.h      | 13 ++++++++++-
 fsck/quotaio_tree.c | 14 +++++++-----
 fsck/quotaio_v2.c   | 53 +++++++++++++++++++++++++++++++++++++++++----
 fsck/segment.c      | 20 +++++++++++++++++
 8 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/fsck/fsck.c b/fsck/fsck.c
index c249dfa..e110f3d 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -792,6 +792,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 
 	if ((node_blk->i.i_inline & F2FS_INLINE_DATA)) {
 		unsigned int inline_size = MAX_INLINE_DATA(node_blk);
+		if (cur_qtype != -1)
+			qf_szchk_type[cur_qtype] = QF_SZCHK_INLINE;
 		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs]);
 
 		if (blkaddr != 0) {
@@ -860,6 +862,15 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 	}
 
 	/* check data blocks in inode */
+	if (cur_qtype != -1) {
+		qf_szchk_type[cur_qtype] = QF_SZCHK_REGFILE;
+		qf_maxsize[cur_qtype] = (ADDRS_PER_INODE(&node_blk->i) +
+				2 * ADDRS_PER_BLOCK(&node_blk->i) +
+				2 * ADDRS_PER_BLOCK(&node_blk->i) *
+				NIDS_PER_BLOCK +
+				(u64) ADDRS_PER_BLOCK(&node_blk->i) *
+				NIDS_PER_BLOCK * NIDS_PER_BLOCK) * F2FS_BLKSIZE;
+	}
 	for (idx = 0; idx < ADDRS_PER_INODE(&node_blk->i);
 						idx++, child.pgofs++) {
 		block_t blkaddr = le32_to_cpu(node_blk->i.i_addr[ofs + idx]);
@@ -884,6 +895,8 @@ void fsck_chk_inode_blk(struct f2fs_sb_info *sbi, u32 nid,
 					file_is_encrypt(&node_blk->i));
 			if (!ret) {
 				*blk_cnt = *blk_cnt + 1;
+				if (cur_qtype != -1 && blkaddr != NEW_ADDR)
+					qf_last_blkofs[cur_qtype] = child.pgofs;
 			} else if (c.fix_on) {
 				node_blk->i.i_addr[ofs + idx] = 0;
 				need_fix = 1;
@@ -1126,6 +1139,8 @@ int fsck_chk_dnode_blk(struct f2fs_sb_info *sbi, struct f2fs_inode *inode,
 			file_is_encrypt(inode));
 		if (!ret) {
 			*blk_cnt = *blk_cnt + 1;
+			if (cur_qtype != -1 && blkaddr != NEW_ADDR)
+				qf_last_blkofs[cur_qtype] = child->pgofs;
 		} else if (c.fix_on) {
 			node_blk->dn.addr[idx] = 0;
 			need_fix = 1;
@@ -1794,6 +1809,7 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
 	u32 blk_cnt = 0;
 
 	for (qtype = 0; qtype < F2FS_MAX_QUOTAS; qtype++) {
+		cur_qtype = qtype;
 		if (sb->qf_ino[qtype] == 0)
 			continue;
 		nid_t ino = QUOTA_INO(sb, qtype);
@@ -1811,10 +1827,13 @@ int fsck_chk_quota_node(struct f2fs_sb_info *sbi)
 		}
 		ret = fsck_chk_node_blk(sbi, NULL, ino,
 				F2FS_FT_REG_FILE, TYPE_INODE, &blk_cnt, NULL);
-		if (ret)
+		if (ret) {
 			ASSERT_MSG("wrong quota inode, qtype [%d] ino [0x%x]",
 								qtype, ino);
+			qf_szchk_type[qtype] = QF_SZCHK_ERR;
+		}
 	}
+	cur_qtype = -1;
 	return ret;
 }
 
diff --git a/fsck/fsck.h b/fsck/fsck.h
index 2de6f62..bc6a435 100644
--- a/fsck/fsck.h
+++ b/fsck/fsck.h
@@ -266,6 +266,8 @@ block_t new_node_block(struct f2fs_sb_info *,
 					struct dnode_of_data *, unsigned int);
 
 /* segment.c */
+struct quota_file;
+u64 f2fs_quota_size(struct quota_file *);
 u64 f2fs_read(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
 u64 f2fs_write(struct f2fs_sb_info *, nid_t, u8 *, u64, pgoff_t);
 void f2fs_filesize_update(struct f2fs_sb_info *, nid_t, u64);
diff --git a/fsck/mkquota.c b/fsck/mkquota.c
index 84f9d3d..c419a0f 100644
--- a/fsck/mkquota.c
+++ b/fsck/mkquota.c
@@ -378,6 +378,7 @@ errcode_t quota_compare_and_update(struct f2fs_sb_info *sbi,
 	err = quota_file_open(sbi, &qh, qtype, 0);
 	if (err) {
 		log_debug("Open quota file failed");
+		*usage_inconsistent = 1;
 		goto out;
 	}
 
diff --git a/fsck/quotaio.c b/fsck/quotaio.c
index cc517bd..51abbb7 100644
--- a/fsck/quotaio.c
+++ b/fsck/quotaio.c
@@ -33,6 +33,14 @@ struct disk_dqheader {
 	__le32 dqh_version;
 } __attribute__ ((packed));
 
+int cur_qtype = -1;
+u32 qf_last_blkofs[MAXQUOTAS] = {0, 0, 0};
+enum qf_szchk_type_t qf_szchk_type[MAXQUOTAS] =
+{
+	QF_SZCHK_NONE, QF_SZCHK_NONE, QF_SZCHK_NONE
+};
+u64 qf_maxsize[MAXQUOTAS];
+
 /**
  * Convert type of quota to written representation
  */
@@ -140,7 +148,7 @@ errcode_t quota_file_open(struct f2fs_sb_info *sbi, struct quota_handle *h,
 		goto errout;
 	}
 
-	if (h->qh_ops->init_io && (h->qh_ops->init_io(h) < 0)) {
+	if (h->qh_ops->init_io && (h->qh_ops->init_io(h, qtype) < 0)) {
 		log_err("qh_ops->init_io failed");
 		err = EIO;
 		goto errout;
diff --git a/fsck/quotaio.h b/fsck/quotaio.h
index 8087309..999e800 100644
--- a/fsck/quotaio.h
+++ b/fsck/quotaio.h
@@ -46,6 +46,17 @@ enum quota_type {
 #error "cannot have more than 32 quota types to fit in qtype_bits"
 #endif
 
+enum qf_szchk_type_t {
+	QF_SZCHK_NONE,
+	QF_SZCHK_ERR,
+	QF_SZCHK_INLINE,
+	QF_SZCHK_REGFILE,
+};
+
+extern int cur_qtype;
+extern u32 qf_last_blkofs[];
+extern enum qf_szchk_type_t qf_szchk_type[];
+extern u64 qf_maxsize[];
 
 #define QUOTA_USR_BIT (1 << USRQUOTA)
 #define QUOTA_GRP_BIT (1 << GRPQUOTA)
@@ -154,7 +165,7 @@ struct quotafile_ops {
 	/* Check whether quotafile is in our format */
 	int (*check_file) (struct quota_handle *h, int type);
 	/* Open quotafile */
-	int (*init_io) (struct quota_handle *h);
+	int (*init_io) (struct quota_handle *h, enum quota_type qtype);
 	/* Create new quotafile */
 	int (*new_io) (struct quota_handle *h);
 	/* Write all changes and close quotafile */
diff --git a/fsck/quotaio_tree.c b/fsck/quotaio_tree.c
index de25a60..c203400 100644
--- a/fsck/quotaio_tree.c
+++ b/fsck/quotaio_tree.c
@@ -568,7 +568,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
 	int entries, i;
 
 	if (!buf)
-		return 0;
+		return -1;
 
 	set_bit(bitmap, blk);
 	read_blk(dquot->dq_h, blk, buf);
@@ -593,9 +593,7 @@ static int report_block(struct dquot *dquot, unsigned int blk, char *bitmap,
 static int check_reference(struct quota_handle *h, unsigned int blk)
 {
 	if (blk >= h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks) {
-		log_err("Illegal reference (%u >= %u) in %s quota file. "
-			"Quota file is probably corrupted.\n"
-			"Please run fsck (8) to fix it.",
+		log_err("Illegal reference (%u >= %u) in %s quota file",
 			blk,
 			h->qh_info.u.v2_mdqi.dqi_qtree.dqi_blocks,
 			quota_type2name(h->qh_type));
@@ -627,9 +625,13 @@ static int report_tree(struct dquot *dquot, unsigned int blk, int depth,
 			break;
 
 		if (depth == QT_TREEDEPTH - 1) {
-			if (!get_bit(bitmap, blk))
-				*entries += report_block(dquot, blk, bitmap,
+			if (!get_bit(bitmap, blk)) {
+				int num_entry = report_block(dquot, blk, bitmap,
 							process_dquot, data);
+				if (num_entry < 0)
+					break;
+				*entries += num_entry;
+			}
 		} else {
 			if (report_tree(dquot, blk, depth + 1, bitmap, entries,
 						process_dquot, data))
diff --git a/fsck/quotaio_v2.c b/fsck/quotaio_v2.c
index 1404332..9353f85 100644
--- a/fsck/quotaio_v2.c
+++ b/fsck/quotaio_v2.c
@@ -20,7 +20,7 @@
 #include "quotaio_tree.h"
 
 static int v2_check_file(struct quota_handle *h, int type);
-static int v2_init_io(struct quota_handle *h);
+static int v2_init_io(struct quota_handle *h, enum quota_type qtype);
 static int v2_new_io(struct quota_handle *h);
 static int v2_write_info(struct quota_handle *h);
 static struct dquot *v2_read_dquot(struct quota_handle *h, qid_t id);
@@ -170,19 +170,64 @@ static int v2_check_file(struct quota_handle *h, int type)
 /*
  * Open quotafile
  */
-static int v2_init_io(struct quota_handle *h)
+static int v2_init_io(struct quota_handle *h, enum quota_type qtype)
 {
 	struct v2_disk_dqinfo ddqinfo;
+	struct v2_mem_dqinfo *info;
+	u64 filesize;
+	struct quota_file *qf = &h->qh_qf;
+	u32 last_blkofs = qf_last_blkofs[qtype];
 
 	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_entry_size =
 		sizeof(struct v2r1_disk_dqblk);
 	h->qh_info.u.v2_mdqi.dqi_qtree.dqi_ops = &v2r1_fmt_ops;
 
 	/* Read information about quotafile */
-	if (h->read(&h->qh_qf, V2_DQINFOOFF, &ddqinfo,
-			 sizeof(ddqinfo)) != sizeof(ddqinfo))
+	if (h->read(qf, V2_DQINFOOFF, &ddqinfo,
+			sizeof(ddqinfo)) != sizeof(ddqinfo))
 		return -1;
 	v2_disk2memdqinfo(&h->qh_info, &ddqinfo);
+
+	/* Check to make sure quota file info is sane */
+	info = &h->qh_info.u.v2_mdqi;
+	filesize = qf->filesize = f2fs_quota_size(qf);
+	if (qf_szchk_type[qtype] == QF_SZCHK_REGFILE &&
+			((filesize + F2FS_BLKSIZE - 1) >> F2FS_BLKSIZE_BITS <
+			last_blkofs + 1 || filesize > qf_maxsize[qtype])) {
+		/*
+		 * reqular: qf_szchk is now the last block index,
+		 * including the hole's index
+		 */
+		log_err("Quota inode %u corrupted: file size %" PRIu64
+			" does not match page offset %" PRIu32,
+			h->qh_qf.ino,
+			filesize,
+			last_blkofs);
+		filesize = (last_blkofs + 1) << F2FS_BLKSIZE_BITS;
+		f2fs_filesize_update(qf->sbi, qf->ino, filesize);
+	}
+
+	if ((info->dqi_qtree.dqi_blocks >
+			(filesize + QT_BLKSIZE - 1) >> QT_BLKSIZE_BITS)) {
+		log_err("Quota inode %u corrupted: file size %" PRId64 "; "
+				"dqi_blocks %u", h->qh_qf.ino,
+				filesize, info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
+	if (info->dqi_qtree.dqi_free_blk >= info->dqi_qtree.dqi_blocks) {
+		log_err("Quota inode %u corrupted: free_blk %u;"
+				" dqi_blocks %u",
+				h->qh_qf.ino, info->dqi_qtree.dqi_free_blk,
+				info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
+	if (info->dqi_qtree.dqi_free_entry >= info->dqi_qtree.dqi_blocks) {
+		log_err("Quota inode %u corrupted: free_entry %u; "
+				"dqi_blocks %u", h->qh_qf.ino,
+				info->dqi_qtree.dqi_free_entry,
+				info->dqi_qtree.dqi_blocks);
+		return -1;
+	}
 	return 0;
 }
 
diff --git a/fsck/segment.c b/fsck/segment.c
index b7cf245..0487f41 100644
--- a/fsck/segment.c
+++ b/fsck/segment.c
@@ -15,6 +15,7 @@
  */
 #include "fsck.h"
 #include "node.h"
+#include "quotaio.h"
 
 int reserve_new_block(struct f2fs_sb_info *sbi, block_t *to,
 			struct f2fs_summary *sum, int type, bool is_inode)
@@ -124,6 +125,25 @@ int new_data_block(struct f2fs_sb_info *sbi, void *block,
 	return 0;
 }
 
+u64 f2fs_quota_size(struct quota_file *qf)
+{
+	struct node_info ni;
+	struct f2fs_node *inode;
+	u64 filesize;
+
+	inode = (struct f2fs_node *) calloc(BLOCK_SZ, 1);
+	ASSERT(inode);
+
+	/* Read inode */
+	get_node_info(qf->sbi, qf->ino, &ni);
+	ASSERT(dev_read_block(inode, ni.blk_addr) >= 0);
+	ASSERT(S_ISREG(le16_to_cpu(inode->i.i_mode)));
+
+	filesize = le64_to_cpu(inode->i.i_size);
+	free(inode);
+	return filesize;
+}
+
 u64 f2fs_read(struct f2fs_sb_info *sbi, nid_t ino, u8 *buffer,
 					u64 count, pgoff_t offset)
 {
-- 
2.27.0.111.gc72c7da667-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel