[U-Boot] [PATCH 1/2] ext4: Avoid corruption of directories with hash tree indexes

[U-Boot] [PATCH 1/2] ext4: Avoid corruption of directories with hash tree indexes

[Stefan Brüns]

While directories can be read using the old linear scan method, adding a
new file would require updating the index tree (alternatively, the whole
tree could be removed).

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_write.c | 5 +++++
 include/ext4fs.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
index 4235b95..ccc8a9c 100644
--- a/fs/ext4/ext4_write.c
+++ b/fs/ext4/ext4_write.c
@@ -864,6 +864,11 @@ int ext4fs_write(const char *fname, unsigned char *buffer,
 		goto fail;
 	if (ext4fs_iget(parent_inodeno, g_parent_inode))
 		goto fail;
+	/* do not mess up a directory using hash trees */
+	if (le32_to_cpu(g_parent_inode->flags) & EXT4_INDEX_FL) {
+		printf("hash tree directory\n");
+		goto fail;
+	}
 	/* check if the filename is already present in root */
 	existing_file_inodeno = ext4fs_filename_unlink(filename);
 	if (existing_file_inodeno != -1) {
diff --git a/include/ext4fs.h b/include/ext4fs.h
index e3f6216..6e31c73 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -28,6 +28,7 @@
 #define __EXT4__
 #include <ext_common.h>
 
+#define EXT4_INDEX_FL		0x00001000 /* Inode uses hash tree index */
 #define EXT4_EXTENTS_FL		0x00080000 /* Inode uses extents */
 #define EXT4_EXT_MAGIC			0xf30a
 #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM	0x0010
-- 
2.9.2

[U-Boot] [PATCH 2/2] ext4: scan all directory blocks when looking up an entry

[Stefan Brüns]

Scanning only the direct blocks of the directory file may falsely report
an existing file as nonexisting, and worse can also lead to creation
of a duplicate entry on file creation.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/ext4/ext4_common.c | 74 +++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 41 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 8d0013e..aa72b0d 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -500,64 +500,53 @@ static int search_dir(struct ext2_inode *parent_inode, char *dirname)
 {
 	int status;
 	int inodeno = 0;
-	int totalbytes;
-	int templength;
-	int direct_blk_idx;
+	int offset;
+	int blk_idx;
 	long int blknr;
-	char *ptr = NULL;
 	unsigned char *block_buffer = NULL;
 	struct ext2_dirent *dir = NULL;
 	struct ext_filesystem *fs = get_fs();
+	uint32_t directory_blocks;
 
-	/* read the block no allocated to a file */
-	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
-		direct_blk_idx++) {
-		blknr = read_allocated_block(parent_inode, direct_blk_idx);
-		if (blknr == 0)
-			goto fail;
+	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
+		LOG2_BLOCK_SIZE(ext4fs_root);
 
-		/* read the blocks of parent inode */
-		block_buffer = zalloc(fs->blksz);
-		if (!block_buffer)
+	block_buffer = zalloc(fs->blksz);
+	if (!block_buffer)
+		goto fail;
+
+	/* get the block no allocated to a file */
+	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
+		blknr = read_allocated_block(parent_inode, blk_idx);
+		if (blknr == 0)
 			goto fail;
 
+		/* read the directory block */
 		status = ext4fs_devread((lbaint_t)blknr * fs->sect_perblk,
 					0, fs->blksz, (char *)block_buffer);
 		if (status == 0)
 			goto fail;
 
+		offset = 0;
 		dir = (struct ext2_dirent *)block_buffer;
-		ptr = (char *)dir;
-		totalbytes = 0;
-		while (le16_to_cpu(dir->direntlen) >= 0) {
-			/*
-			 * blocksize-totalbytes because last directory
-			 * length i.e.,*dir->direntlen is free availble
-			 * space in the block that means
-			 * it is a last entry of directory entry
-			 */
-			if (dir->inode && (strlen(dirname) == dir->namelen)) {
-				if (strncmp(dirname, ptr + sizeof(struct ext2_dirent), dir->namelen) == 0) {
-					inodeno = le32_to_cpu(dir->inode);
-					break;
-				}
+		while (le16_to_cpu(dir->direntlen) >= 8) {
+			if (dir->inode && (strlen(dirname) == dir->namelen) &&
+			    (strncmp(dirname, &dir[1], dir->namelen) == 0)) {
+				inodeno = le32_to_cpu(dir->inode);
+				break;
 			}
 
-			if (fs->blksz - totalbytes == le16_to_cpu(dir->direntlen))
+			offset += le16_to_cpu(dir->direntlen);
+			if (offset >= fs->blksz)
 				break;
 
-			/* traversing the each directory entry */
-			templength = le16_to_cpu(dir->direntlen);
-			totalbytes = totalbytes + templength;
-			dir = (struct ext2_dirent *)((char *)dir + templength);
-			ptr = (char *)dir;
+			dir = (struct ext2_dirent *)(block_buffer + offset);
 		}
 
-		free(block_buffer);
-		block_buffer = NULL;
-
-		if (inodeno > 0)
+		if (inodeno > 0) {
+			free(block_buffer);
 			return inodeno;
+		}
 	}
 
 fail:
@@ -809,14 +798,17 @@ fail:
 
 int ext4fs_filename_unlink(char *filename)
 {
-	short direct_blk_idx = 0;
+	int blk_idx;
 	long int blknr = -1;
 	int inodeno = -1;
+	uint32_t directory_blocks;
+
+	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
+		LOG2_BLOCK_SIZE(ext4fs_root);
 
 	/* read the block no allocated to a file */
-	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
-		direct_blk_idx++) {
-		blknr = read_allocated_block(g_parent_inode, direct_blk_idx);
+	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
+		blknr = read_allocated_block(g_parent_inode, blk_idx);
 		if (blknr == 0)
 			break;
 		inodeno = unlink_filename(filename, blknr);
-- 
2.9.2

[U-Boot] [PATCH 1/2] ext4: Avoid corruption of directories with hash tree indexes

[Lukasz Majewski]

Hi Stefan,

> While directories can be read using the old linear scan method,
> adding a new file would require updating the index tree
> (alternatively, the whole tree could be removed).
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_write.c | 5 +++++
>  include/ext4fs.h     | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> index 4235b95..ccc8a9c 100644
> --- a/fs/ext4/ext4_write.c
> +++ b/fs/ext4/ext4_write.c
> @@ -864,6 +864,11 @@ int ext4fs_write(const char *fname, unsigned
> char *buffer, goto fail;
>  	if (ext4fs_iget(parent_inodeno, g_parent_inode))
>  		goto fail;
> +	/* do not mess up a directory using hash trees */
> +	if (le32_to_cpu(g_parent_inode->flags) & EXT4_INDEX_FL) {
> +		printf("hash tree directory\n");
> +		goto fail;
> +	}
>  	/* check if the filename is already present in root */
>  	existing_file_inodeno = ext4fs_filename_unlink(filename);
>  	if (existing_file_inodeno != -1) {
> diff --git a/include/ext4fs.h b/include/ext4fs.h
> index e3f6216..6e31c73 100644
> --- a/include/ext4fs.h
> +++ b/include/ext4fs.h
> @@ -28,6 +28,7 @@
>  #define __EXT4__
>  #include <ext_common.h>
>  
> +#define EXT4_INDEX_FL		0x00001000 /* Inode uses hash
> tree index */ #define EXT4_EXTENTS_FL		0x00080000 /*
> Inode uses extents */ #define EXT4_EXT_MAGIC
> 0xf30a #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM	0x0010

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 2/2] ext4: scan all directory blocks when looking up an entry

[Lukasz Majewski]

Hi Stefan,

> Scanning only the direct blocks of the directory file may falsely
> report an existing file as nonexisting, and worse can also lead to
> creation of a duplicate entry on file creation.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/ext4/ext4_common.c | 74
> +++++++++++++++++++++++---------------------------- 1 file changed,
> 33 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 8d0013e..aa72b0d 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -500,64 +500,53 @@ static int search_dir(struct ext2_inode
> *parent_inode, char *dirname) {
>  	int status;
>  	int inodeno = 0;
> -	int totalbytes;
> -	int templength;
> -	int direct_blk_idx;
> +	int offset;
> +	int blk_idx;
>  	long int blknr;
> -	char *ptr = NULL;
>  	unsigned char *block_buffer = NULL;
>  	struct ext2_dirent *dir = NULL;
>  	struct ext_filesystem *fs = get_fs();
> +	uint32_t directory_blocks;
>  
> -	/* read the block no allocated to a file */
> -	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
> -		direct_blk_idx++) {
> -		blknr = read_allocated_block(parent_inode,
> direct_blk_idx);
> -		if (blknr == 0)
> -			goto fail;
> +	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
> +		LOG2_BLOCK_SIZE(ext4fs_root);
>  
> -		/* read the blocks of parent inode */
> -		block_buffer = zalloc(fs->blksz);
> -		if (!block_buffer)
> +	block_buffer = zalloc(fs->blksz);
> +	if (!block_buffer)
> +		goto fail;
> +
> +	/* get the block no allocated to a file */
> +	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
> +		blknr = read_allocated_block(parent_inode, blk_idx);
> +		if (blknr == 0)
>  			goto fail;
>  
> +		/* read the directory block */
>  		status = ext4fs_devread((lbaint_t)blknr *
> fs->sect_perblk, 0, fs->blksz, (char *)block_buffer);
>  		if (status == 0)
>  			goto fail;
>  
> +		offset = 0;
>  		dir = (struct ext2_dirent *)block_buffer;
> -		ptr = (char *)dir;
> -		totalbytes = 0;
> -		while (le16_to_cpu(dir->direntlen) >= 0) {
> -			/*
> -			 * blocksize-totalbytes because last
> directory
> -			 * length i.e.,*dir->direntlen is free
> availble
> -			 * space in the block that means
> -			 * it is a last entry of directory entry
> -			 */
> -			if (dir->inode && (strlen(dirname) ==
> dir->namelen)) {
> -				if (strncmp(dirname, ptr +
> sizeof(struct ext2_dirent), dir->namelen) == 0) {
> -					inodeno =
> le32_to_cpu(dir->inode);
> -					break;
> -				}
> +		while (le16_to_cpu(dir->direntlen) >= 8) {
> +			if (dir->inode && (strlen(dirname) ==
> dir->namelen) &&
> +			    (strncmp(dirname, &dir[1], dir->namelen)
> == 0)) {
> +				inodeno = le32_to_cpu(dir->inode);
> +				break;
>  			}
>  
> -			if (fs->blksz - totalbytes ==
> le16_to_cpu(dir->direntlen))
> +			offset += le16_to_cpu(dir->direntlen);
> +			if (offset >= fs->blksz)
>  				break;
>  
> -			/* traversing the each directory entry */
> -			templength = le16_to_cpu(dir->direntlen);
> -			totalbytes = totalbytes + templength;
> -			dir = (struct ext2_dirent *)((char *)dir +
> templength);
> -			ptr = (char *)dir;
> +			dir = (struct ext2_dirent *)(block_buffer +
> offset); }
>  
> -		free(block_buffer);
> -		block_buffer = NULL;
> -
> -		if (inodeno > 0)
> +		if (inodeno > 0) {
> +			free(block_buffer);
>  			return inodeno;
> +		}
>  	}
>  
>  fail:
> @@ -809,14 +798,17 @@ fail:
>  
>  int ext4fs_filename_unlink(char *filename)
>  {
> -	short direct_blk_idx = 0;
> +	int blk_idx;
>  	long int blknr = -1;
>  	int inodeno = -1;
> +	uint32_t directory_blocks;
> +
> +	directory_blocks = le32_to_cpu(g_parent_inode->size) >>
> +		LOG2_BLOCK_SIZE(ext4fs_root);
>  
>  	/* read the block no allocated to a file */
> -	for (direct_blk_idx = 0; direct_blk_idx < INDIRECT_BLOCKS;
> -		direct_blk_idx++) {
> -		blknr = read_allocated_block(g_parent_inode,
> direct_blk_idx);
> +	for (blk_idx = 0; blk_idx < directory_blocks; blk_idx++) {
> +		blknr = read_allocated_block(g_parent_inode,
> blk_idx); if (blknr == 0)
>  			break;
>  		inodeno = unlink_filename(filename, blknr);

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group