There appears to be the potential for an undetected error in fs/ext2/dev.c ext2fs_devread()

U-Boot Quentin Armitage 2 participants 2 messages Jan 3, 2010 – Jan 17, 2010 ext2 file system

Share & tools

Quentin Armitage

author

2010-01-03 01:30 UTC #0 lore raw

In fs/ext2/dev.c in function ext2fs_devread(), if after reading the first part, there is less than a whole block left to be read (block_len == 0), then the return value from ext2fs_block_dev_desc->block_read is not tested, and success (1) is always returned to the calling function.

This particular block of code appears superfluous, since there is already code to read the final part, and so the attached patch slightly restructures the code to use that, and deletes the block of code that does not check the return value.

The second assignment of block_len = byte_len & ~(SECTOR_SIZE - 1); (after the read of the main block) is unnecessary since the same assignment has already been made 21 lines above.

The first change in the patch is purely cosmetic, in that it simply changes an error message to make it consistent with the other error messages in the function.

ext2/dev.c 2010-01-03 01:08:43.000000000 +000 renamed from fs/ext2/dev.c +13 ~~-23~~

@@ -84,7 +84,7 @@ int ext2fs_devread (int sector, int byte
 		    block_read (ext2fs_block_dev_desc->dev,
 				part_info.start + sector, 1,
 				(unsigned long *) sec_buf) != 1) {
-			printf (" ** ext2fs_devread() read error **\n");
+			printf (" ** ext2fs_devread() read error - first part\n");
 			return (0);
 		}
 		memcpy (buf, sec_buf + byte_offset,

@@ -100,29 +100,19 @@ int ext2fs_devread (int sector, int byte
 	/*  read sector aligned part */
 	block_len = byte_len & ~(SECTOR_SIZE - 1);
 
-	if (block_len == 0) {
-		u8 p[SECTOR_SIZE];
-
-		block_len = SECTOR_SIZE;
-		ext2fs_block_dev_desc->block_read(ext2fs_block_dev_desc->dev,
-						  part_info.start + sector,
-						  1, (unsigned long *)p);
-		memcpy(buf, p, byte_len);
-		return 1;
-	}
-
-	if (ext2fs_block_dev_desc->block_read (ext2fs_block_dev_desc->dev,
-					       part_info.start + sector,
-					       block_len / SECTOR_SIZE,
-					       (unsigned long *) buf) !=
-	    block_len / SECTOR_SIZE) {
-		printf (" ** ext2fs_devread() read error - block\n");
-		return (0);
+	if (block_len > 0) {
+		if (ext2fs_block_dev_desc->block_read (ext2fs_block_dev_desc->dev,
+						       part_info.start + sector,
+						       block_len / SECTOR_SIZE,
+						       (unsigned long *) buf) !=
+		    block_len / SECTOR_SIZE) {
+			printf (" ** ext2fs_devread() read error - block\n");
+			return (0);
+		}
+		buf += block_len;
+		byte_len -= block_len;
+		sector += block_len / SECTOR_SIZE;
 	}
-	block_len = byte_len & ~(SECTOR_SIZE - 1);
-	buf += block_len;
-	byte_len -= block_len;
-	sector += block_len / SECTOR_SIZE;
 
 	if (byte_len != 0) {
 		/* read rest of data which are not in whole sector */

Regards,
Quentin Armitage

Wolfgang Denk

2010-01-17 23:33 UTC #1 lore raw

Dear Quentin Armitage,

Quoted 4 lines · In message <[email protected]> you wrote:

In fs/ext2/dev.c in function ext2fs_devread(), if after reading the first part, there is less than a whole block left to be read (block_len == 0), then the return value from ext2fs_block_dev_desc->block_read is not tested, and success (1) is always returned to the calling function.

Do you have a test case (for example, a file system image) that can be used to test this error (i. e. that shows the error with the current code, and that works with your patch)?

What exactly are the symptoms of the error?

ext2/dev.c 2010-01-03 01:08:43.000000000 +000

Quoted 15 lines

This particular block of code appears superfluous, since there is already code to read the final part, and so the attached patch slightly restructures the code to use that, and deletes the block of code that does not check the return value.
The second assignment of block_len = byte_len & ~(SECTOR_SIZE - 1); (after the read of the main block) is unnecessary since the same assignment has already been made 21 lines above.
The first change in the patch is purely cosmetic, in that it simply changes an error message to make it consistent with the other error messages in the function.
ext2/dev.c 2010-01-03 01:08:43.000000000 +000 renamed from fs/ext2/dev.c +0 -0

Signed-off-by line missing.

Best regards,

Wolfgang Denk

Signature · 5 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Unser Kopf ist rund, damit das Denken die Richtung wechseln kann.
                                                   -- Francis Picabia