Re: [U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Lukasz Majewski]

Hi Stefan,

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Brüns 
> ---
>  fs/ext4/ext4_common.c | 45
> + 1 file changed, 17
> insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
> index 699a640..11be0b7 100644
> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> @@ -854,16 +854,15 @@ fail:
>  
>  static int unlink_filename(char *filename, unsigned int blknr)
>  {
> - int templength = 0;
> - int status, inodeno;
> - int found = 0;
> + int status;
> + int inodeno = 0;
>   int offset;
>   char *block_buffer = NULL;
>   struct ext2_dirent *dir = NULL;
> - struct ext2_dirent *previous_dir = NULL;
> - char *ptr = NULL;
> + struct ext2_dirent *previous_dir;
>   struct ext_filesystem *fs = get_fs();
>   int ret = -1;
> + char *direntname;
>  
>   block_buffer = zalloc(fs->blksz);
>   if (!block_buffer)
> @@ -877,20 +876,18 @@ static int unlink_filename(char *filename,
> unsigned int blknr) 
>   if (ext4fs_log_journal(block_buffer, blknr))
>   goto fail;
> - dir = (struct ext2_dirent *)block_buffer;
> - ptr = (char *)dir;
>   offset = 0;
> - while (le16_to_cpu(dir->direntlen) >= 0) {
> - /*
> -  * blocksize-offset 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
> -  */
> + do {
> + previous_dir = dir;
> + dir = (struct ext2_dirent *)(block_buffer + offset);
> + direntname = (char *)(dir) + sizeof(struct
> ext2_dirent); +
> + int direntlen = le16_to_cpu(dir->direntlen);
> + if (direntlen < sizeof(struct ext2_dirent))
> + break;
> +
>   if (dir->inode && (strlen(filename) == dir->namelen)
> &&
> - (strncmp(ptr + sizeof(struct ext2_dirent),
> -  filename, dir->namelen) == 0)) {
> - printf("file found, deleting\n");
> + (strncmp(direntname, filename, dir->namelen) ==
> 0)) { inodeno = le32_to_cpu(dir->inode);
>   if (previous_dir) {
>   uint16_t new_len;
> @@ -900,23 +897,15 @@ static int unlink_filename(char *filename,
> unsigned int blknr) } else {
>   dir->inode = 0;
>   }
> - found = 1;
>   break;
>   }
>  
> - if (fs->blksz - offset ==
> le16_to_cpu(dir->direntlen))
> - break;
> + offset += direntlen;
>  
> - /* traversing the each directory entry */
> - templength = le16_to_cpu(dir->direntlen);
> - offset = offset + templength;
> - previous_dir = dir;
> - dir = (struct ext2_dirent *)((char *)dir +
> templength);
> - ptr = (char *)dir;
> - }
> + } while (offset < fs->blksz);
>  
> + if (inodeno > 0) {
>  
> - if (found == 1) {
>   if (ext4fs_put_metadata(block_buffer, blknr))
>   goto fail;
>   ret = inodeno;

Reviewed-by: Lukasz Majewski 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Tom Rini]

On Sun, Oct 09, 2016 at 08:15:27PM +0200, Stefan Brüns wrote:

> The direntlen checks were quite bogus, i.e. the loop termination used
> "len + offset == blocksize" (exact match only), and checked for a
> direntlen less than 0. The latter can never happen as the len is
> unsigned, this has been reported by Coverity, CID 153384.
> 
> Use the same code as in search_dir for directory traversal. This code
> has the correct checks for direntlen >= sizeof(struct dirent), and
> offset < blocksize.
> 
> Signed-off-by: Stefan Brüns 

Reported-by: Coverity (CID: 153383, 153384)
Reviewed-by: Tom Rini 

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/3] ext4: Fix handling of direntlen in unlink_filename

[Stefan Brüns]

The direntlen checks were quite bogus, i.e. the loop termination used
"len + offset == blocksize" (exact match only), and checked for a
direntlen less than 0. The latter can never happen as the len is
unsigned, this has been reported by Coverity, CID 153384.

Use the same code as in search_dir for directory traversal. This code
has the correct checks for direntlen >= sizeof(struct dirent), and
offset < blocksize.

Signed-off-by: Stefan Brüns 
---
 fs/ext4/ext4_common.c | 45 +
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c
index 699a640..11be0b7 100644
--- a/fs/ext4/ext4_common.c
+++ b/fs/ext4/ext4_common.c
@@ -854,16 +854,15 @@ fail:
 
 static int unlink_filename(char *filename, unsigned int blknr)
 {
-   int templength = 0;
-   int status, inodeno;
-   int found = 0;
+   int status;
+   int inodeno = 0;
int offset;
char *block_buffer = NULL;
struct ext2_dirent *dir = NULL;
-   struct ext2_dirent *previous_dir = NULL;
-   char *ptr = NULL;
+   struct ext2_dirent *previous_dir;
struct ext_filesystem *fs = get_fs();
int ret = -1;
+   char *direntname;
 
block_buffer = zalloc(fs->blksz);
if (!block_buffer)
@@ -877,20 +876,18 @@ static int unlink_filename(char *filename, unsigned int 
blknr)
 
if (ext4fs_log_journal(block_buffer, blknr))
goto fail;
-   dir = (struct ext2_dirent *)block_buffer;
-   ptr = (char *)dir;
offset = 0;
-   while (le16_to_cpu(dir->direntlen) >= 0) {
-   /*
-* blocksize-offset 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
-*/
+   do {
+   previous_dir = dir;
+   dir = (struct ext2_dirent *)(block_buffer + offset);
+   direntname = (char *)(dir) + sizeof(struct ext2_dirent);
+
+   int direntlen = le16_to_cpu(dir->direntlen);
+   if (direntlen < sizeof(struct ext2_dirent))
+   break;
+
if (dir->inode && (strlen(filename) == dir->namelen) &&
-   (strncmp(ptr + sizeof(struct ext2_dirent),
-filename, dir->namelen) == 0)) {
-   printf("file found, deleting\n");
+   (strncmp(direntname, filename, dir->namelen) == 0)) {
inodeno = le32_to_cpu(dir->inode);
if (previous_dir) {
uint16_t new_len;
@@ -900,23 +897,15 @@ static int unlink_filename(char *filename, unsigned int 
blknr)
} else {
dir->inode = 0;
}
-   found = 1;
break;
}
 
-   if (fs->blksz - offset == le16_to_cpu(dir->direntlen))
-   break;
+   offset += direntlen;
 
-   /* traversing the each directory entry */
-   templength = le16_to_cpu(dir->direntlen);
-   offset = offset + templength;
-   previous_dir = dir;
-   dir = (struct ext2_dirent *)((char *)dir + templength);
-   ptr = (char *)dir;
-   }
+   } while (offset < fs->blksz);
 
+   if (inodeno > 0) {
 
-   if (found == 1) {
if (ext4fs_put_metadata(block_buffer, blknr))
goto fail;
ret = inodeno;
-- 
2.10.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot