[U-Boot] FAT12 regression after 8d48c92b45aea91e2a2be90f2ed93677e85526f1

[U-Boot] FAT12 regression after 8d48c92b45aea91e2a2be90f2ed93677e85526f1

[Oleksandr Tymoshenko]

Hello,

U-Boot 2017.01 and master branch is broken on BeagleBone Black
with boot partition formatted as FAT12, it hang after printing "Loading
u-boot.img" message. I bisected regression to this patch:

http://lists.denx.de/pipermail/u-boot/2016-December/276305.html

This code simplification is not going to work on architectures
with strict alignment requirements:

+               ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));

fatbuf is a pointer to __u8 and off16 can take any values so
mydata->fatbuf + off16 is not guaranteed to be 16-bits aligned
and 16-bit access to non-aligned address will cause exception.

-- 
gonzo

[U-Boot] FAT12 regression after 8d48c92b45aea91e2a2be90f2ed93677e85526f1

[Heiko Schocher]

Hello Oleksandr,

added Stefan and Tom to cc...

Am 26.01.2017 um 00:25 schrieb Oleksandr Tymoshenko:
> Hello,
>
> U-Boot 2017.01 and master branch is broken on BeagleBone Black
> with boot partition formatted as FAT12, it hang after printing "Loading
> u-boot.img" message. I bisected regression to this patch:
>
> http://lists.denx.de/pipermail/u-boot/2016-December/276305.html

Yep, I detected this issue for another am335x based board, too,
see thread here:'
http://lists.denx.de/pipermail/u-boot/2017-January/279078.html

But I did not found time to look into it...

> This code simplification is not going to work on architectures
> with strict alignment requirements:
>
> +               ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));
>
> fatbuf is a pointer to __u8 and off16 can take any values so
> mydata->fatbuf + off16 is not guaranteed to be 16-bits aligned
> and 16-bit access to non-aligned address will cause exception.

Good explanation! Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] fs/fat: Fix unaligned __u16 reads for FAT12 access

[Brüns, Stefan]

Doing unaligned reads is not supported on all architectures, use
byte sized reads of the little endian buffer.
Rename off16 to off8, as it reflects the buffer offset in byte
granularity (offset is in entry, i.e. 12 bit, granularity).
Fix a regression introduced in 8d48c92b45aea91e2a2be90f2ed93677e85526f1

Reported-by: Oleksandr Tymoshenko <gonzo@bluezbox.com>
Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 fs/fat/fat.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index fe899d0442..06088e2421 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -179,7 +179,7 @@ int flush_dirty_fat_buffer(fsdata *mydata)
 static __u32 get_fatent(fsdata *mydata, __u32 entry)
 {
 	__u32 bufnum;
-	__u32 off16, offset;
+	__u32 offset, off8;
 	__u32 ret = 0x00;
 
 	if (CHECK_CLUST(entry, mydata->fatsize)) {
@@ -242,8 +242,9 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)
 		ret = FAT2CPU16(((__u16 *) mydata->fatbuf)[offset]);
 		break;
 	case 12:
-		off16 = (offset * 3) / 2;
-		ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));
+		off8 = (offset * 3) / 2;
+		/* fatbut + off8 may be unaligned, read in byte granularity */
+		ret = mydata->fatbuf[off8] + mydata->fatbuf[off8 + 1] << 8;
 
 		if (offset & 0x1)
 			ret >>= 4;
-- 
2.11.0

[U-Boot] [PATCH] fs/fat: Fix unaligned __u16 reads for FAT12 access

[Oleksandr Tymoshenko]

Br?ns, Stefan (Stefan.Bruens at rwth-aachen.de) wrote:
> Doing unaligned reads is not supported on all architectures, use
> byte sized reads of the little endian buffer.
> Rename off16 to off8, as it reflects the buffer offset in byte
> granularity (offset is in entry, i.e. 12 bit, granularity).
> Fix a regression introduced in 8d48c92b45aea91e2a2be90f2ed93677e85526f1
> 
> Reported-by: Oleksandr Tymoshenko <gonzo@bluezbox.com>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  fs/fat/fat.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index fe899d0442..06088e2421 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -179,7 +179,7 @@ int flush_dirty_fat_buffer(fsdata *mydata)
>  static __u32 get_fatent(fsdata *mydata, __u32 entry)
>  {
>  	__u32 bufnum;
> -	__u32 off16, offset;
> +	__u32 offset, off8;
>  	__u32 ret = 0x00;
>  
>  	if (CHECK_CLUST(entry, mydata->fatsize)) {
> @@ -242,8 +242,9 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)
>  		ret = FAT2CPU16(((__u16 *) mydata->fatbuf)[offset]);
>  		break;
>  	case 12:
> -		off16 = (offset * 3) / 2;
> -		ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));
> +		off8 = (offset * 3) / 2;
> +		/* fatbut + off8 may be unaligned, read in byte granularity */
> +		ret = mydata->fatbuf[off8] + mydata->fatbuf[off8 + 1] << 8;

This patch when applied gives me "Invalid FAT entry" message. Reason
is: operator '<<' has lower precedence than '+' so this expression is
equivalent to (mydata->fatbuf[off8] + mydata->fatbuf[off8 + 1]) << 8
With explicit parentheses around shift it works as expected:

ret = mydata->fatbuf[off8] + (mydata->fatbuf[off8 + 1] << 8);

-- 
gonzo