* [U-Boot] [PATCH 0/2] fs: fat: error handling in get_contents()
@ 2019-09-12 17:19 Heinrich Schuchardt
2019-09-12 17:19 ` [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors Heinrich Schuchardt
2019-09-12 17:19 ` [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors Heinrich Schuchardt
0 siblings, 2 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2019-09-12 17:19 UTC (permalink / raw)
To: u-boot
If a FAT entry is corrupted, reading a file should always lead to an
error.
get_contents() should always return -1 when an error occurs.
Heinrich Schuchardt (2):
fs: fat: treat invalid FAT clusters as errors
fs: fat: get_contents() always returns -1 for errors
fs/fat/fat.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors
2019-09-12 17:19 [U-Boot] [PATCH 0/2] fs: fat: error handling in get_contents() Heinrich Schuchardt
@ 2019-09-12 17:19 ` Heinrich Schuchardt
2019-09-18 4:37 ` AKASHI Takahiro
2019-10-12 20:28 ` Tom Rini
2019-09-12 17:19 ` [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors Heinrich Schuchardt
1 sibling, 2 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2019-09-12 17:19 UTC (permalink / raw)
To: u-boot
When hitting an invalid FAT cluster while reading a file always print an
error message and return an error code.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors
2019-09-12 17:19 [U-Boot] [PATCH 0/2] fs: fat: error handling in get_contents() Heinrich Schuchardt
2019-09-12 17:19 ` [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors Heinrich Schuchardt
@ 2019-09-12 17:19 ` Heinrich Schuchardt
2019-10-12 20:28 ` Tom Rini
1 sibling, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2019-09-12 17:19 UTC (permalink / raw)
To: u-boot
If out of memory, return -1 and not -ENOMEM from get_contents().
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
fs/fat/fat.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index b4e8083734..da822f4f38 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -364,7 +364,7 @@ static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos,
tmp_buffer = malloc_cache_aligned(actsize);
if (!tmp_buffer) {
debug("Error: allocating buffer\n");
- return -ENOMEM;
+ return -1;
}
if (get_cluster(mydata, curclust, tmp_buffer, actsize) != 0) {
--
2.23.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors
2019-09-12 17:19 ` [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors Heinrich Schuchardt
@ 2019-09-18 4:37 ` AKASHI Takahiro
2019-10-12 20:28 ` Tom Rini
1 sibling, 0 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2019-09-18 4:37 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors
2019-09-12 17:19 ` [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors Heinrich Schuchardt
2019-09-18 4:37 ` AKASHI Takahiro
@ 2019-10-12 20:28 ` Tom Rini
1 sibling, 0 replies; 6+ messages in thread
From: Tom Rini @ 2019-10-12 20:28 UTC (permalink / raw)
To: u-boot
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.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191012/c36d26d1/attachment.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors
2019-09-12 17:19 ` [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors Heinrich Schuchardt
@ 2019-10-12 20:28 ` Tom Rini
0 siblings, 0 replies; 6+ messages in thread
From: Tom Rini @ 2019-10-12 20:28 UTC (permalink / raw)
To: u-boot
On Thu, Sep 12, 2019 at 07:19:30PM +0200, Heinrich Schuchardt wrote:
> If out of memory, return -1 and not -ENOMEM from get_contents().
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191012/861d49eb/attachment.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-12 20:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-09-12 17:19 [U-Boot] [PATCH 0/2] fs: fat: error handling in get_contents() Heinrich Schuchardt
2019-09-12 17:19 ` [U-Boot] [PATCH 1/2] fs: fat: treat invalid FAT clusters as errors Heinrich Schuchardt
2019-09-18 4:37 ` AKASHI Takahiro
2019-10-12 20:28 ` Tom Rini
2019-09-12 17:19 ` [U-Boot] [PATCH 2/2] fs: fat: get_contents() always returns -1 for errors Heinrich Schuchardt
2019-10-12 20:28 ` Tom Rini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox