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

[Quentin Armitage]

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.

--- orig/fs/ext2/dev.c	2010-01-03 01:04:57.000000000 +0000
+++ fs/ext2/dev.c       2010-01-03 01:08:43.000000000 +000
@@ -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