Re: [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors

On Thu, Sep 12, 2019 at 07:19:29PM +0200, Heinrich Schuchardt wrote:

When hitting an invalid FAT cluster while reading a file always print an error message and return an error code.

I don't know what the intention of original author was here.

In general, a cluster's FAT entry points to a next cluster which composes part of a file and hence a chain of clusters. The last cluster's FAT is marked with EOC(End of Cluster), which has a dedicated value, for fat16, one of 0xfff8:0xffff.

As you can see, those values are also detected by CHECK_CLUST() macro. I'm afraid that, at least initially, this macro might have worked for detecting a file end correctly (so returned 0?).

Another issue is that CHECK_CLUST() checks against >0xfff0 for fat16 while 0xfff0:0xfff6 are still valid for cluster numbers. So it will potentially detect a wrong "invalid" FAT entry for a quite large file.

Thanks, -Takahiro Akashi

Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de

fs/fat/fat.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 73a89fc9e3..b4e8083734 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -301,10 +301,20 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size) return 0; }

-/* +/**

• • get_contents() - read from file
• • Read at most 'maxsize' bytes from 'pos' in the file associated with 'dentptr'

• • into 'buffer'.
• • Update the number of bytes read in *gotsize or return -1 on fatal errors.

• • into 'buffer'. Update the number of bytes read in *gotsize or return -1 on
• • fatal errors.
• • @mydata: file system description
• • @dentprt: directory entry pointer
• • @pos: position from where to read
• • @buffer: buffer into which to read
• • @maxsize: maximum number of bytes to read
• • @gotsize: number of bytes actually read
• • Return: -1 on error, otherwise 0
  */

static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer, loff_t maxsize, loff_t *gotsize) @@ -335,8 +345,8 @@ static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, curclust = get_fatent(mydata, curclust); if (CHECK_CLUST(curclust, mydata->fatsize)) { debug("curclust: 0x%x\n", curclust);

• 	debug("Invalid FAT entry\n");
  

• 	return 0;
  

• 	printf("Invalid FAT entry\n");
  

• 	return -1;
  

  } actsize += bytesperclust; }

@@ -374,8 +384,8 @@ static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, curclust = get_fatent(mydata, curclust); if (CHECK_CLUST(curclust, mydata->fatsize)) { debug("curclust: 0x%x\n", curclust);

• 	debug("Invalid FAT entry\n");
  

• 	return 0;
  

• 	printf("Invalid FAT entry\n");
  

• 	return -1;
  

  } }

@@ -390,8 +400,8 @@ static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, goto getit; if (CHECK_CLUST(newclust, mydata->fatsize)) { debug("curclust: 0x%x\n", newclust);

• 		debug("Invalid FAT entry\n");
  

• 		return 0;
  

• 		printf("Invalid FAT entry\n");
  

• 		return -1;
  }
  endclust = newclust;
  actsize += bytesperclust;
  

@@ -418,7 +428,7 @@ getit: if (CHECK_CLUST(curclust, mydata->fatsize)) { debug("curclust: 0x%x\n", curclust); printf("Invalid FAT entry\n");

• 	return 0;
  

• 	return -1;
  

  } actsize = bytesperclust; endclust = curclust;

-- 2.23.0