ext4: Verify fast symlink length

PATCH Applied Jan Kara 5 participants 7 messages Feb 6, 2025 – Mar 18, 2025 ext4 file system

Share & tools

Jan Kara

author reviewer

2025-02-06 09:45 UTC #0 lore raw

PATCH 1/1 ext4: Verify fast symlink length

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.

Reported-by [email protected]

Tested-by [email protected]

Fixes bae80473f7b0 ("ext4: use inode_set_cached_link()")

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/inode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

fs/ext4/inode.c +10 -2

index 7c54ae5fcbd4..64e280fed911 100644
@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||
+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {
+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);
 		} else {

Signature · 1 line

2.43.0

Theodore Ts'o

maintainer

2025-02-06 15:24 UTC #1 lore raw

Quoted 24 lines · Jan Kara

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.
Reported-by [email protected]
Tested-by [email protected]
Fixes bae80473f7b0 ("ext4: use inode_set_cached_link()")
Suggested-by "Darrick J. Wong" <[email protected]>
Signed-off-by Jan Kara <[email protected]>
---
 fs/ext4/inode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
fs/ext4/inode.c +2 -2
index 7c54ae5fcbd4..64e280fed911 100644
@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||

fs/ext4/inode.c struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, · line 5021

Quoted 10 lines

fs/ext4/inode.c +8 -0

+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {
+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);

I don't think this will do the right thing if the fast symlink is encrypted. See ext4_encrypted_get_link() in fs/ext4/symlink.c in the kernel sources, and also look at how e2fsck_pass1_check_symlink() handles checking the size of an encrypted, fast symlink.

Thanks,

- Ted

Eric Biggers

2025-02-06 16:04 UTC #2 lore raw

Quoted 33 lines · Theodore Ts'o

Quoted 32 lines · Jan Kara

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.

Reported-by [email protected]

Tested-by [email protected]

Fixes bae80473f7b0 ("ext4: use inode_set_cached_link()")

Signed-off-by Jan Kara <[email protected]>

---
 fs/ext4/inode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

fs/ext4/inode.c +10 -2

index 7c54ae5fcbd4..64e280fed911 100644
@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||
+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {
+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}

Quoted 9 lines

fs/ext4/inode.c struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, · line 5021
Quoted 2 lines
fs/ext4/inode.c +0 -0
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);
I don't think this will do the right thing if the fast symlink is encrypted. See ext4_encrypted_get_link() in fs/ext4/symlink.c in the kernel sources, and also look at how e2fsck_pass1_check_symlink() handles checking the size of an encrypted, fast symlink.

Encrypted symlinks are handled separately just a couple lines above.

- Eric

Jan Kara

author reviewer

2025-02-07 12:54 UTC #3 lore raw

In reply to Theodore Ts'o

Quoted 21 lines · Theodore Ts'o

Quoted 20 lines · Jan Kara

fs/ext4/inode.c +10 -2

index 7c54ae5fcbd4..64e280fed911 100644
@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||
+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {
+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}

Quoted 8 lines

fs/ext4/inode.c struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, · line 5021
Quoted 2 lines
fs/ext4/inode.c +0 -0
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);
I don't think this will do the right thing if the fast symlink is encrypted. See ext4_encrypted_get_link() in fs/ext4/symlink.c in the kernel sources, and also look at how e2fsck_pass1_check_symlink() handles checking the size of an encrypted, fast symlink.

Thanks for having a look but as Eric writes, there's:

                if (IS_ENCRYPTED(inode)) {
                        inode->i_op = &ext4_encrypted_symlink_inode_operations;
		} else if (ext4_inode_is_fast_symlink(inode)) {

just above the context of this hunk so I *think* encrypted symlinks cannot get into this code?

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Darrick J. Wong

Reviewed-by on v1 1/1

2025-02-06 15:34 UTC #4 lore raw

In reply to Jan Kara

Quoted 17 lines · Jan Kara

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.
Reported-by [email protected]
Tested-by [email protected]
Fixes bae80473f7b0 ("ext4: use inode_set_cached_link()")
Suggested-by "Darrick J. Wong" <[email protected]>
Signed-off-by Jan Kara <[email protected]>
---
 fs/ext4/inode.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
fs/ext4/inode.c +0 -0
index 7c54ae5fcbd4..64e280fed911 100644

fs/ext4/inode.c struct inode *__ext4_iget(struct super_block *sb, unsigned long ino, · line 5013

Quoted 9 lines

fs/ext4/inode.c +4 -2

@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
 			inode->i_op = &ext4_encrypted_symlink_inode_operations;
 		} else if (ext4_inode_is_fast_symlink(inode)) {
 			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||
+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {

Oooh, a validation I overlooked! :D

Reviewed-by "Darrick J. Wong" <[email protected]>

--D

Quoted 12 lines

fs/ext4/inode.c +6 -0

+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}
 			inode_set_cached_link(inode, (char *)ei->i_data,
 					      inode->i_size);
 		} else {

Signature · 1 line

2.43.0

Baokun Li

Reviewed-by on v1 1/1

2025-02-10 13:51 UTC #5 lore raw

In reply to Jan Kara

Quoted 9 lines · Jan Kara

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.
Reported-by [email protected]
Tested-by [email protected]
Fixes bae80473f7b0 ("ext4: use inode_set_cached_link()")
Suggested-by "Darrick J. Wong" <[email protected]>
Signed-off-by Jan Kara <[email protected]>
---

Looks good to me, thanks for the patch!

Reviewed-by Baokun Li <[email protected]>

Quoted 26 lines

  fs/ext4/inode.c | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

fs/ext4/inode.c +10 -2

index 7c54ae5fcbd4..64e280fed911 100644
@@ -5007,8 +5007,16 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
  			inode->i_op = &ext4_encrypted_symlink_inode_operations;
  		} else if (ext4_inode_is_fast_symlink(inode)) {
  			inode->i_op = &ext4_fast_symlink_inode_operations;
-			nd_terminate_link(ei->i_data, inode->i_size,
-				sizeof(ei->i_data) - 1);
+			if (inode->i_size == 0 ||
+			    inode->i_size >= sizeof(ei->i_data) ||
+			    strnlen((char *)ei->i_data, inode->i_size + 1) !=
+								inode->i_size) {
+				ext4_error_inode(inode, function, line, 0,
+					"invalid fast symlink length %llu",
+					 (unsigned long long)inode->i_size);
+				ret = -EFSCORRUPTED;
+				goto bad_inode;
+			}
  			inode_set_cached_link(inode, (char *)ei->i_data,
  					      inode->i_size);
  		} else {

Theodore Ts'o

maintainer Applied

2025-03-18 03:42 UTC #6 lore raw

In reply to Jan Kara

Quoted 4 lines · Jan Kara

Verify fast symlink length stored in inode->i_size matches the string stored in the inode to avoid surprises from corrupted filesystems.

Applied, thanks!

[1/1] ext4: Verify fast symlink length

      commit: 5f920d5d60839f7cbbb1ed50eac68d8bc0a73a7c

Best regards,

Signature · 1 line

Theodore Ts'o <[email protected]>