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

[Quentin Armitage]

On Mon, 2010-01-18 at 00:33 +0100, Wolfgang Denk wrote: 
> Dear Quentin Armitage,
> 
> In message <1262482223.2820.161.camel at samson.armitage.org.uk> 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?
> 
The issue was determined by code inspection (I was reading it to understand an unrelated issue).
Within ext2fs_devread(), all except one of the calls to
ext2fs_block_dev_desc->block_read() check the return code, the exception
being the second call to the block_read function. This would occur if no
complete sectors are being read, in other words: (byte_len - byte_offset
% SECTOR_SIZE) < SECTOR_SIZE.

The symptoms of the error would be that the calling function would not
be aware that there had been a read error if one occurred, since success
(1) is always returned after that call to block_read().

As identified below, this particular block of code is unnecessary, and
the patch below, which slightly simplifies the code, also removes the
issue.

Below is the original email with a Signed-off-by line added:

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 */

Signed-off-by: Quentin Armitage<Quentin at Armitage.org.uk>

Regards,
Quentin Armitage