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

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 stefan.bruens@rwth-aachen.de --- 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;

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 stefan.bruens@rwth-aachen.de
Reported-by: Coverity (CID: 153383, 153384) Reviewed-by: Tom Rini trini@konsulko.com

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 stefan.bruens@rwth-aachen.de
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 l.majewski@samsung.com

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 stefan.bruens@rwth-aachen.de Reported-by: Coverity (CID: 153383, 153384) Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Lukasz Majewski l.majewski@samsung.com
Applied to u-boot/master, thanks!
participants (3)
-
Lukasz Majewski
-
Stefan Brüns
-
Tom Rini