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

[Chao Yu <yuchao0@huawei.com>]

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