[U-Boot] [PATCH] FAT: buffer overflow with FAT12/16

[U-Boot] [PATCH] FAT: buffer overflow with FAT12/16

[Stefano Babic]

Last commit 3831530dcb7b71329c272ccd6181f8038b6a6dd0a was intended
"explicitly specify FAT12/16 root directory parsing buffer size, instead
of relying on cluster size". Howver, the underlying function requires
the size of the buffer in blocks, not in bytes, and instead of passing
a double sector size a request for 1024 blocks is sent. This generates
a buffer overflow with overwriting of other structure (in the case seen,
USB structures were overwritten).

Signed-off-by: Stefano Babic <sbabic@denx.de>
CC: Mikhail Zolotaryov <lebon@lebon.org.ua>

---
 fs/fat/fat.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index 744e961..a75e4f2 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -858,7 +858,7 @@ do_fat_read (const char *filename, void *buffer, unsigned long maxsize,
 		if (disk_read(cursect,
 				(mydata->fatsize == 32) ?
 				(mydata->clust_size) :
-				LINEAR_PREFETCH_SIZE,
+				LINEAR_PREFETCH_SIZE / SECTOR_SIZE,
 				do_fat_read_block) < 0) {
 			debug("Error: reading rootdir block\n");
 			return -1;
-- 
1.7.1

[U-Boot] [PATCH] FAT: buffer overflow with FAT12/16

[Wolfgang Denk]

Dear Stefano Babic,

In message <1287557505-3955-1-git-send-email-sbabic@denx.de> you wrote:
> Last commit 3831530dcb7b71329c272ccd6181f8038b6a6dd0a was intended
> "explicitly specify FAT12/16 root directory parsing buffer size, instead
> of relying on cluster size". Howver, the underlying function requires
> the size of the buffer in blocks, not in bytes, and instead of passing
> a double sector size a request for 1024 blocks is sent. This generates
> a buffer overflow with overwriting of other structure (in the case seen,
> USB structures were overwritten).
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> CC: Mikhail Zolotaryov <lebon@lebon.org.ua>
> 
> ---
>  fs/fat/fat.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Nice catch!

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
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
Never worry about theory as long as  the  machinery  does  what  it's
supposed to do.                                      - R. A. Heinlein

[U-Boot] [PATCH] FAT: buffer overflow with FAT12/16

[Mikhail Zolotaryov]

 Stefano,

you're right, sorry for that. Isn't it better to define LINEAR_PREFETCH_SIZE as
2 (blocks) and change get_vfatname() to something like that in such case:
        __u8 *buflimit = cluster + ((curclust == 0) ?
                                        LINEAR_PREFETCH_SIZE :
                                        mydata->clust_size
                                   ) * SECTOR_SIZE;
Regards,
Mikhail

On 20.10.10 09:51, Stefano Babic wrote:
> Last commit 3831530dcb7b71329c272ccd6181f8038b6a6dd0a was intended
> "explicitly specify FAT12/16 root directory parsing buffer size, instead
> of relying on cluster size". Howver, the underlying function requires
> the size of the buffer in blocks, not in bytes, and instead of passing
> a double sector size a request for 1024 blocks is sent. This generates
> a buffer overflow with overwriting of other structure (in the case seen,
> USB structures were overwritten).
>
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> CC: Mikhail Zolotaryov <lebon@lebon.org.ua>
>
> ---
>  fs/fat/fat.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 744e961..a75e4f2 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -858,7 +858,7 @@ do_fat_read (const char *filename, void *buffer, unsigned long maxsize,
>  		if (disk_read(cursect,
>  				(mydata->fatsize == 32) ?
>  				(mydata->clust_size) :
> -				LINEAR_PREFETCH_SIZE,
> +				LINEAR_PREFETCH_SIZE / SECTOR_SIZE,
>  				do_fat_read_block) < 0) {
>  			debug("Error: reading rootdir block\n");
>  			return -1;