f2fs: fix to disallow getting inner inode via f2fs_iget()

PATCH In review Chao Yu 2 participants 10 messages Aug 30, 2022 – Sep 13, 2022 F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2022-08-30 22:54 UTC #0 lore raw

PATCH v2 1/1 f2fs: fix to disallow getting inner inode via f2fs_iget()

From: Chao Yu <[email protected]>

Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.

Signed-off-by Chao Yu <[email protected]>

---
v2:
- don't override errno from f2fs_check_nid_range()
- fix compile error

 fs/f2fs/compress.c |  2 +-
 fs/f2fs/f2fs.h     |  1 +
 fs/f2fs/inode.c    | 13 ++++++++++++-
 fs/f2fs/super.c    |  4 ++--
 4 files changed, 16 insertions(+), 4 deletions(-)

fs/f2fs/compress.c +1 -1

index 730256732a9e..c38b22bb6432 100644
@@ -1947,7 +1947,7 @@ int f2fs_init_compress_inode(struct f2fs_sb_info *sbi)
 	if (!test_opt(sbi, COMPRESS_CACHE))
 		return 0;
 
-	inode = f2fs_iget(sbi->sb, F2FS_COMPRESS_INO(sbi));
+	inode = f2fs_iget_inner(sbi->sb, F2FS_COMPRESS_INO(sbi));
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 	sbi->compress_inode = inode;

fs/f2fs/f2fs.h +1 -0

index 408d8034ed74..35f9e1a6a1bf 100644
@@ -3467,6 +3467,7 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
 void f2fs_set_inode_flags(struct inode *inode);
 bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
 void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino);
 struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
 struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
 int f2fs_try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);

fs/f2fs/inode.c +12 -1

index 6d11c365d7b4..965f87c1dd63 100644
@@ -480,7 +480,7 @@ static int do_read_inode(struct inode *inode)
 	return 0;
 }
 
-struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	struct inode *inode;

@@ -568,6 +568,17 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 	return ERR_PTR(ret);
 }
 
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);
+}
+
 struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
 {
 	struct inode *inode;

fs/f2fs/super.c +2 -2

index b8e5fe244596..a5f5e7483791 100644
@@ -4157,7 +4157,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_xattr_cache;
 
 	/* get an inode for meta space */
-	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
+	sbi->meta_inode = f2fs_iget_inner(sb, F2FS_META_INO(sbi));
 	if (IS_ERR(sbi->meta_inode)) {
 		f2fs_err(sbi, "Failed to read F2FS meta data inode");
 		err = PTR_ERR(sbi->meta_inode);

@@ -4265,7 +4265,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_nm;
 
 	/* get an inode for node space */
-	sbi->node_inode = f2fs_iget(sb, F2FS_NODE_INO(sbi));
+	sbi->node_inode = f2fs_iget_inner(sb, F2FS_NODE_INO(sbi));
 	if (IS_ERR(sbi->node_inode)) {
 		f2fs_err(sbi, "Failed to read node inode");
 		err = PTR_ERR(sbi->node_inode);

Signature · 1 line

2.25.1

Chao Yu

author maintainer

2022-09-08 01:59 UTC #1 lore raw

Ping,

Quoted 95 lines · Chao Yu

From: Chao Yu <[email protected]>

Signed-off-by Chao Yu <[email protected]>

---
v2:
- don't override errno from f2fs_check_nid_range()
- fix compile error

  fs/f2fs/compress.c |  2 +-
  fs/f2fs/f2fs.h     |  1 +
  fs/f2fs/inode.c    | 13 ++++++++++++-
  fs/f2fs/super.c    |  4 ++--
  4 files changed, 16 insertions(+), 4 deletions(-)

fs/f2fs/compress.c +1 -1

index 730256732a9e..c38b22bb6432 100644
@@ -1947,7 +1947,7 @@ int f2fs_init_compress_inode(struct f2fs_sb_info *sbi)
  	if (!test_opt(sbi, COMPRESS_CACHE))
  		return 0;
 
-	inode = f2fs_iget(sbi->sb, F2FS_COMPRESS_INO(sbi));
+	inode = f2fs_iget_inner(sbi->sb, F2FS_COMPRESS_INO(sbi));
  	if (IS_ERR(inode))
  		return PTR_ERR(inode);
  	sbi->compress_inode = inode;

fs/f2fs/f2fs.h +1 -0

index 408d8034ed74..35f9e1a6a1bf 100644
@@ -3467,6 +3467,7 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
  void f2fs_set_inode_flags(struct inode *inode);
  bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
  void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
  int f2fs_try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);

fs/f2fs/inode.c +12 -1

index 6d11c365d7b4..965f87c1dd63 100644
@@ -480,7 +480,7 @@ static int do_read_inode(struct inode *inode)
  	return 0;
  }
 
-struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino)
  {
  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
  	struct inode *inode;

@@ -568,6 +568,17 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
  	return ERR_PTR(ret);
  }
 
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);
+}
+
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
  {
  	struct inode *inode;

fs/f2fs/super.c +2 -2

index b8e5fe244596..a5f5e7483791 100644
@@ -4157,7 +4157,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_xattr_cache;
 
  	/* get an inode for meta space */
-	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
+	sbi->meta_inode = f2fs_iget_inner(sb, F2FS_META_INO(sbi));
  	if (IS_ERR(sbi->meta_inode)) {
  		f2fs_err(sbi, "Failed to read F2FS meta data inode");
  		err = PTR_ERR(sbi->meta_inode);

@@ -4265,7 +4265,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_nm;
 
  	/* get an inode for node space */
-	sbi->node_inode = f2fs_iget(sb, F2FS_NODE_INO(sbi));
+	sbi->node_inode = f2fs_iget_inner(sb, F2FS_NODE_INO(sbi));
  	if (IS_ERR(sbi->node_inode)) {
  		f2fs_err(sbi, "Failed to read node inode");
  		err = PTR_ERR(sbi->node_inode);

Jaegeuk Kim

maintainer

2022-09-08 02:02 UTC #2 lore raw

In reply to Chao Yu

Quoted 5 lines · Chao Yu

From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.

So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?

Quoted 92 lines

Signed-off-by Chao Yu <[email protected]>

---
v2:
- don't override errno from f2fs_check_nid_range()
- fix compile error

 fs/f2fs/compress.c |  2 +-
 fs/f2fs/f2fs.h     |  1 +
 fs/f2fs/inode.c    | 13 ++++++++++++-
 fs/f2fs/super.c    |  4 ++--
 4 files changed, 16 insertions(+), 4 deletions(-)

fs/f2fs/compress.c +1 -1

index 730256732a9e..c38b22bb6432 100644
@@ -1947,7 +1947,7 @@ int f2fs_init_compress_inode(struct f2fs_sb_info *sbi)
 	if (!test_opt(sbi, COMPRESS_CACHE))
 		return 0;
 
-	inode = f2fs_iget(sbi->sb, F2FS_COMPRESS_INO(sbi));
+	inode = f2fs_iget_inner(sbi->sb, F2FS_COMPRESS_INO(sbi));
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 	sbi->compress_inode = inode;

fs/f2fs/f2fs.h +1 -0

index 408d8034ed74..35f9e1a6a1bf 100644
@@ -3467,6 +3467,7 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
 void f2fs_set_inode_flags(struct inode *inode);
 bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
 void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino);
 struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
 struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
 int f2fs_try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);

fs/f2fs/inode.c +12 -1

index 6d11c365d7b4..965f87c1dd63 100644
@@ -480,7 +480,7 @@ static int do_read_inode(struct inode *inode)
 	return 0;
 }
 
-struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(sb);
 	struct inode *inode;

@@ -568,6 +568,17 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 	return ERR_PTR(ret);
 }
 
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);
+}
+
 struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
 {
 	struct inode *inode;

fs/f2fs/super.c +2 -2

index b8e5fe244596..a5f5e7483791 100644
@@ -4157,7 +4157,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_xattr_cache;
 
 	/* get an inode for meta space */
-	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
+	sbi->meta_inode = f2fs_iget_inner(sb, F2FS_META_INO(sbi));
 	if (IS_ERR(sbi->meta_inode)) {
 		f2fs_err(sbi, "Failed to read F2FS meta data inode");
 		err = PTR_ERR(sbi->meta_inode);

@@ -4265,7 +4265,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 		goto free_nm;
 
 	/* get an inode for node space */
-	sbi->node_inode = f2fs_iget(sb, F2FS_NODE_INO(sbi));
+	sbi->node_inode = f2fs_iget_inner(sb, F2FS_NODE_INO(sbi));
 	if (IS_ERR(sbi->node_inode)) {
 		f2fs_err(sbi, "Failed to read node inode");
 		err = PTR_ERR(sbi->node_inode);

Signature · 1 line

2.25.1

Chao Yu

author maintainer

2022-09-08 02:11 UTC #3 lore raw

Quoted 9 lines · Jaegeuk Kim

Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?

For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:

	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}

compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?

Thanks,

Quoted 93 lines

Signed-off-by Chao Yu <[email protected]>

---
v2:
- don't override errno from f2fs_check_nid_range()
- fix compile error

  fs/f2fs/compress.c |  2 +-
  fs/f2fs/f2fs.h     |  1 +
  fs/f2fs/inode.c    | 13 ++++++++++++-
  fs/f2fs/super.c    |  4 ++--
  4 files changed, 16 insertions(+), 4 deletions(-)

fs/f2fs/compress.c +1 -1

index 730256732a9e..c38b22bb6432 100644
@@ -1947,7 +1947,7 @@ int f2fs_init_compress_inode(struct f2fs_sb_info *sbi)
  	if (!test_opt(sbi, COMPRESS_CACHE))
  		return 0;
 
-	inode = f2fs_iget(sbi->sb, F2FS_COMPRESS_INO(sbi));
+	inode = f2fs_iget_inner(sbi->sb, F2FS_COMPRESS_INO(sbi));
  	if (IS_ERR(inode))
  		return PTR_ERR(inode);
  	sbi->compress_inode = inode;

fs/f2fs/f2fs.h +1 -0

index 408d8034ed74..35f9e1a6a1bf 100644
@@ -3467,6 +3467,7 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
  void f2fs_set_inode_flags(struct inode *inode);
  bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
  void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
  int f2fs_try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);

fs/f2fs/inode.c +12 -1

index 6d11c365d7b4..965f87c1dd63 100644
@@ -480,7 +480,7 @@ static int do_read_inode(struct inode *inode)
  	return 0;
  }
 
-struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino)
  {
  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
  	struct inode *inode;

@@ -568,6 +568,17 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
  	return ERR_PTR(ret);
  }
 
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);
+}
+
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
  {
  	struct inode *inode;

fs/f2fs/super.c +2 -2

index b8e5fe244596..a5f5e7483791 100644
@@ -4157,7 +4157,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_xattr_cache;
 
  	/* get an inode for meta space */
-	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
+	sbi->meta_inode = f2fs_iget_inner(sb, F2FS_META_INO(sbi));
  	if (IS_ERR(sbi->meta_inode)) {
  		f2fs_err(sbi, "Failed to read F2FS meta data inode");
  		err = PTR_ERR(sbi->meta_inode);

@@ -4265,7 +4265,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_nm;
 
  	/* get an inode for node space */
-	sbi->node_inode = f2fs_iget(sb, F2FS_NODE_INO(sbi));
+	sbi->node_inode = f2fs_iget_inner(sb, F2FS_NODE_INO(sbi));
  	if (IS_ERR(sbi->node_inode)) {
  		f2fs_err(sbi, "Failed to read node inode");
  		err = PTR_ERR(sbi->node_inode);

Signature · 1 line

2.25.1

Jaegeuk Kim

maintainer

2022-09-08 02:19 UTC #4 lore raw

Quoted 25 lines · Chao Yu

Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}

Quoted 2 lines

compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?

Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?

Quoted 83 lines

Thanks,

Quoted 80 lines

Signed-off-by Chao Yu <[email protected]>

---
v2:
- don't override errno from f2fs_check_nid_range()
- fix compile error

  fs/f2fs/compress.c |  2 +-
  fs/f2fs/f2fs.h     |  1 +
  fs/f2fs/inode.c    | 13 ++++++++++++-
  fs/f2fs/super.c    |  4 ++--
  4 files changed, 16 insertions(+), 4 deletions(-)

fs/f2fs/compress.c +1 -1

index 730256732a9e..c38b22bb6432 100644
@@ -1947,7 +1947,7 @@ int f2fs_init_compress_inode(struct f2fs_sb_info *sbi)
  	if (!test_opt(sbi, COMPRESS_CACHE))
  		return 0;
-	inode = f2fs_iget(sbi->sb, F2FS_COMPRESS_INO(sbi));
+	inode = f2fs_iget_inner(sbi->sb, F2FS_COMPRESS_INO(sbi));
  	if (IS_ERR(inode))
  		return PTR_ERR(inode);
  	sbi->compress_inode = inode;

fs/f2fs/f2fs.h +1 -0

index 408d8034ed74..35f9e1a6a1bf 100644
@@ -3467,6 +3467,7 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
  void f2fs_set_inode_flags(struct inode *inode);
  bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
  void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
  int f2fs_try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);

fs/f2fs/inode.c +12 -1

index 6d11c365d7b4..965f87c1dd63 100644
@@ -480,7 +480,7 @@ static int do_read_inode(struct inode *inode)
  	return 0;
  }
-struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+struct inode *f2fs_iget_inner(struct super_block *sb, unsigned long ino)
  {
  	struct f2fs_sb_info *sbi = F2FS_SB(sb);
  	struct inode *inode;

@@ -568,6 +568,17 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
  	return ERR_PTR(ret);
  }
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);
+}
+
  struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
  {
  	struct inode *inode;

fs/f2fs/super.c +1 -1

index b8e5fe244596..a5f5e7483791 100644
@@ -4157,7 +4157,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_xattr_cache;
  	/* get an inode for meta space */
-	sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
+	sbi->meta_inode = f2fs_iget_inner(sb, F2FS_META_INO(sbi));
  	if (IS_ERR(sbi->meta_inode)) {
  		f2fs_err(sbi, "Failed to read F2FS meta data inode");
  		err = PTR_ERR(sbi->meta_inode);

@@ -4265,7 +4265,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
  		goto free_nm;

Quoted 8 lines

fs/f2fs/super.c +1 -1

  	/* get an inode for node space */
-	sbi->node_inode = f2fs_iget(sb, F2FS_NODE_INO(sbi));
+	sbi->node_inode = f2fs_iget_inner(sb, F2FS_NODE_INO(sbi));
  	if (IS_ERR(sbi->node_inode)) {
  		f2fs_err(sbi, "Failed to read node inode");
  		err = PTR_ERR(sbi->node_inode);

Signature · 1 line

2.25.1

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

author maintainer

2022-09-08 04:04 UTC #5 lore raw

Quoted 26 lines · Jaegeuk Kim

Quoted 25 lines · Chao Yu
Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}

Quoted 5 lines

Quoted 2 lines
compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?
Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?

Well, the main concern is previously f2fs_iget() won't check validation for inner inode due to it will skip do_read_inode() - f2fs_check_nid_range(), so that in a fuzzed image, caller may pass inner ino into f2fs_iget(), result in incorrect use of inner inode. So I add f2fs_check_nid_range() in prior to f2fs_iget_inner() in f2fs_iget() as below to detect and avoid this case.

Quoted 9 lines

fs/f2fs/inode.c +9 -0

+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);

Jaegeuk Kim

maintainer

2022-09-12 15:39 UTC #6 lore raw

Quoted 30 lines · Chao Yu

Quoted 29 lines · Jaegeuk Kim
Quoted 27 lines · Chao Yu
Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}
compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?

Quoted 8 lines

Quoted 2 lines
Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?
Well, the main concern is previously f2fs_iget() won't check validation for inner inode due to it will skip do_read_inode() - f2fs_check_nid_range(), so that in a fuzzed image, caller may pass inner ino into f2fs_iget(), result in incorrect use of inner inode. So I add f2fs_check_nid_range() in prior to f2fs_iget_inner() in f2fs_iget() as below to detect and avoid this case.

FWIW, sanity_check_raw_super() checked the inode numbers.

Quoted 10 lines

fs/f2fs/inode.c +9 -0

+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);

Chao Yu

author maintainer

2022-09-13 01:26 UTC #7 lore raw

Quoted 34 lines · Jaegeuk Kim

Quoted 33 lines · Chao Yu
Quoted 31 lines · Jaegeuk Kim
Quoted 25 lines · Chao Yu
Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}
Quoted 2 lines
compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?
Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?

Quoted 7 lines

Quoted 5 lines
Well, the main concern is previously f2fs_iget() won't check validation for inner inode due to it will skip do_read_inode() - f2fs_check_nid_range(), so that in a fuzzed image, caller may pass inner ino into f2fs_iget(), result in incorrect use of inner inode. So I add f2fs_check_nid_range() in prior to f2fs_iget_inner() in f2fs_iget() as below to detect and avoid this case.
FWIW, sanity_check_raw_super() checked the inode numbers.

However, previously, f2fs_iget() will return inner inode to caller directly, if caller passes meta_ino, node_ino or compress_ino to f2fs_iget()?

Thanks,

Quoted 11 lines

fs/f2fs/inode.c +9 -0

+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);

Jaegeuk Kim

maintainer

2022-09-13 05:54 UTC #8 lore raw

Quoted 41 lines · Chao Yu

Quoted 40 lines · Jaegeuk Kim
Quoted 38 lines · Chao Yu
Quoted 26 lines · Jaegeuk Kim
Quoted 25 lines · Chao Yu
Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}
Quoted 5 lines
Quoted 2 lines
compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?
Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?
Well, the main concern is previously f2fs_iget() won't check validation for inner inode due to it will skip do_read_inode() - f2fs_check_nid_range(), so that in a fuzzed image, caller may pass inner ino into f2fs_iget(), result in incorrect use of inner inode. So I add f2fs_check_nid_range() in prior to f2fs_iget_inner() in f2fs_iget() as below to detect and avoid this case.

Quoted 4 lines

Quoted 1 line
FWIW, sanity_check_raw_super() checked the inode numbers.
However, previously, f2fs_iget() will return inner inode to caller directly, if caller passes meta_ino, node_ino or compress_ino to f2fs_iget()?

Do you want to do sanity check on corrupted dentry? If so, how about checking it in f2fs_iget instead?

	if (is_meta_ino(ino)) {
		if (!(inode->i_state & I_NEW) 
			return -EFSCORRUPTED;
		goto make_now;
	}

Quoted 5 lines

Thanks,
Quoted 2 lines
Quoted 2 lines

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
(quoted diff) +9 -0
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

author maintainer

2022-09-13 06:44 UTC #9 lore raw

Quoted 42 lines · Jaegeuk Kim

Quoted 41 lines · Chao Yu
Quoted 40 lines · Jaegeuk Kim
Quoted 38 lines · Chao Yu
Quoted 26 lines · Jaegeuk Kim
Quoted 25 lines · Chao Yu
Quoted 9 lines · Jaegeuk Kim
Quoted 5 lines · Chao Yu
From: Chao Yu <[email protected]>
Introduce f2fs_iget_inner() for f2fs_fill_super() to get inner inode:
meta inode, node inode or compressed inode, and add f2fs_check_nid_range() in f2fs_iget() to avoid getting inner inode from external interfaces.
So, we don't want to check the range of inner inode numbers? What'd be the way to check it's okay?
For node_ino, meta_ino, root_ino, we have checked them in sanity_check_raw_super() as below:
	/* check reserved ino info */
	if (le32_to_cpu(raw_super->node_ino) != 1 ||
		le32_to_cpu(raw_super->meta_ino) != 2 ||
		le32_to_cpu(raw_super->root_ino) != 3) {
		f2fs_info(sbi, "Invalid Fs Meta Ino: node(%u) meta(%u) root(%u)",
			  le32_to_cpu(raw_super->node_ino),
			  le32_to_cpu(raw_super->meta_ino),
			  le32_to_cpu(raw_super->root_ino));
		return -EFSCORRUPTED;
	}
Quoted 5 lines
Quoted 2 lines
compressed_ino should always be NM_I(sbi)->max_nid, it can be checked in f2fs_init_compress_inode()?
Hmm, I'm not sure whether we really need this patch, since it'd look better to handle all the iget with single f2fs_iget?
Well, the main concern is previously f2fs_iget() won't check validation for inner inode due to it will skip do_read_inode() - f2fs_check_nid_range(), so that in a fuzzed image, caller may pass inner ino into f2fs_iget(), result in incorrect use of inner inode. So I add f2fs_check_nid_range() in prior to f2fs_iget_inner() in f2fs_iget() as below to detect and avoid this case.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
FWIW, sanity_check_raw_super() checked the inode numbers.
However, previously, f2fs_iget() will return inner inode to caller directly, if caller passes meta_ino, node_ino or compress_ino to f2fs_iget()?
Do you want to do sanity check on corrupted dentry? If so, how about checking

Yes, including:

- corrupted ino in dentry
- corrupted ino of orphan inode

As I replied in this thread:

https://lore.kernel.org/lkml/[email protected]/

Quoted 7 lines

it in f2fs_iget instead?

	if (is_meta_ino(ino)) {
		if (!(inode->i_state & I_NEW)
			return -EFSCORRUPTED;
		goto make_now;
	}

Fine to me, let me revise in v3.

Thanks,

Quoted 15 lines

Quoted 15 lines
Thanks,
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +9 -0
+struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
+{
+	int ret;
+
+	ret = f2fs_check_nid_range(F2FS_SB(sb), ino);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return f2fs_iget_inner(sb, ino);