[PATCH] uimage: fix CRC32 verification on NFS

[PATCH] uimage: fix CRC32 verification on NFS

[Ahmad Fatoum]

Reading a file over NFS is prone to return short reads as the file
content is split over multiple UDP packets and reads won't return
more than the number of bytes that have gathered in the FIFO.

The uImage verification code didn't account for this and handled neither
short reads or the file prematurely ending.

Address both to fix this unexpected result:

  uimage -v /mnt/nfs/uImage
  verifying data CRC... Bad Data CRC: 0x56474aa2 != 0x6b8f0a9c

  cp /mnt/nfs/uImage .
  uimage -v uImage
  verifying data CRC... ok

Fixes: 390249968c4e ("reimplement uImage code")
Closes: https://github.com/barebox/barebox/issues/28
Reported-by: Rashidwi <rashidwinter@gmail.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/uimage.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/uimage.c b/common/uimage.c
index 140a08c1e426..c578edae2606 100644
--- a/common/uimage.c
+++ b/common/uimage.c
@@ -272,7 +272,9 @@ int uimage_verify(struct uimage_handle *handle)
 		ret = read(handle->fd, buf, now);
 		if (ret < 0)
 			goto err;
-		crc = crc32(crc, buf, now);
+		if (!ret)
+			break;
+		crc = crc32(crc, buf, ret);
 		len -= ret;
 	}
 
-- 
2.39.5

Re: [PATCH] uimage: fix CRC32 verification on NFS

[Sascha Hauer]

On Wed, Oct 02, 2024 at 06:07:15PM +0200, Ahmad Fatoum wrote:
> Reading a file over NFS is prone to return short reads as the file
> content is split over multiple UDP packets and reads won't return
> more than the number of bytes that have gathered in the FIFO.
> 
> The uImage verification code didn't account for this and handled neither
> short reads or the file prematurely ending.

Short reads shouldn't be a problem in the code you are touching here.
Have you moved this part to "uimage: use read_full where appropriate"
and didn't adjust the commit message?

> 
> Address both to fix this unexpected result:
> 
>   uimage -v /mnt/nfs/uImage
>   verifying data CRC... Bad Data CRC: 0x56474aa2 != 0x6b8f0a9c
> 
>   cp /mnt/nfs/uImage .
>   uimage -v uImage
>   verifying data CRC... ok
> 
> Fixes: 390249968c4e ("reimplement uImage code")
> Closes: https://github.com/barebox/barebox/issues/28
> Reported-by: Rashidwi <rashidwinter@gmail.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/uimage.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common/uimage.c b/common/uimage.c
> index 140a08c1e426..c578edae2606 100644
> --- a/common/uimage.c
> +++ b/common/uimage.c
> @@ -272,7 +272,9 @@ int uimage_verify(struct uimage_handle *handle)
>  		ret = read(handle->fd, buf, now);
>  		if (ret < 0)
>  			goto err;
> -		crc = crc32(crc, buf, now);
> +		if (!ret)
> +			break;

Should we have an extra error message in this case? The information that
a uImage is shorter than expected might be valuable for the user.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] uimage: fix CRC32 verification on NFS

[Ahmad Fatoum]

On 14.10.24 15:10, Sascha Hauer wrote:
> On Wed, Oct 02, 2024 at 06:07:15PM +0200, Ahmad Fatoum wrote:
>> Reading a file over NFS is prone to return short reads as the file
>> content is split over multiple UDP packets and reads won't return
>> more than the number of bytes that have gathered in the FIFO.
>>
>> The uImage verification code didn't account for this and handled neither
>> short reads or the file prematurely ending.
> 
> Short reads shouldn't be a problem in the code you are touching here.
> Have you moved this part to "uimage: use read_full where appropriate"
> and didn't adjust the commit message?

It's a problem, because the CRC was done on `now' bytes, which is how
many bytes the code would like to read, not `ret', which is the actual
number of bytes read.

>> diff --git a/common/uimage.c b/common/uimage.c
>> index 140a08c1e426..c578edae2606 100644
>> --- a/common/uimage.c
>> +++ b/common/uimage.c
>> @@ -272,7 +272,9 @@ int uimage_verify(struct uimage_handle *handle)
>>  		ret = read(handle->fd, buf, now);
>>  		if (ret < 0)
>>  			goto err;
>> -		crc = crc32(crc, buf, now);
>> +		if (!ret)
>> +			break;
> 
> Should we have an extra error message in this case? The information that
> a uImage is shorter than expected might be valuable for the user.

I don't see the need to differentiate between premature end and corrupted
bytes. Both are problems in another layer anyway and people still stuck
using uImage may not like losing extra bytes for an error message anyway.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] uimage: fix CRC32 verification on NFS

[Sascha Hauer]

On Mon, Oct 14, 2024 at 03:18:18PM +0200, Ahmad Fatoum wrote:
> On 14.10.24 15:10, Sascha Hauer wrote:
> > On Wed, Oct 02, 2024 at 06:07:15PM +0200, Ahmad Fatoum wrote:
> >> Reading a file over NFS is prone to return short reads as the file
> >> content is split over multiple UDP packets and reads won't return
> >> more than the number of bytes that have gathered in the FIFO.
> >>
> >> The uImage verification code didn't account for this and handled neither
> >> short reads or the file prematurely ending.
> > 
> > Short reads shouldn't be a problem in the code you are touching here.
> > Have you moved this part to "uimage: use read_full where appropriate"
> > and didn't adjust the commit message?
> 
> It's a problem, because the CRC was done on `now' bytes, which is how
> many bytes the code would like to read, not `ret', which is the actual
> number of bytes read.

Ah, I missed that you changed the crc32 length argument from 'now' to
'ret'.

> 
> >> diff --git a/common/uimage.c b/common/uimage.c
> >> index 140a08c1e426..c578edae2606 100644
> >> --- a/common/uimage.c
> >> +++ b/common/uimage.c
> >> @@ -272,7 +272,9 @@ int uimage_verify(struct uimage_handle *handle)
> >>  		ret = read(handle->fd, buf, now);
> >>  		if (ret < 0)
> >>  			goto err;
> >> -		crc = crc32(crc, buf, now);
> >> +		if (!ret)
> >> +			break;
> > 
> > Should we have an extra error message in this case? The information that
> > a uImage is shorter than expected might be valuable for the user.
> 
> I don't see the need to differentiate between premature end and corrupted
> bytes. Both are problems in another layer anyway and people still stuck
> using uImage may not like losing extra bytes for an error message anyway.

I could imagine that knowing that the image is too short would put me on
another path when searching for the issue.

Anyway, debugging this shouldn't be too hard, even without this
information.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] uimage: fix CRC32 verification on NFS

[Ahmad Fatoum]

On 14.10.24 16:52, Sascha Hauer wrote:
> On Mon, Oct 14, 2024 at 03:18:18PM +0200, Ahmad Fatoum wrote:
>> On 14.10.24 15:10, Sascha Hauer wrote:
>>> On Wed, Oct 02, 2024 at 06:07:15PM +0200, Ahmad Fatoum wrote:
>>>> diff --git a/common/uimage.c b/common/uimage.c
>>>> index 140a08c1e426..c578edae2606 100644
>>>> --- a/common/uimage.c
>>>> +++ b/common/uimage.c
>>>> @@ -272,7 +272,9 @@ int uimage_verify(struct uimage_handle *handle)
>>>>  		ret = read(handle->fd, buf, now);
>>>>  		if (ret < 0)
>>>>  			goto err;
>>>> -		crc = crc32(crc, buf, now);
>>>> +		if (!ret)
>>>> +			break;
>>>
>>> Should we have an extra error message in this case? The information that
>>> a uImage is shorter than expected might be valuable for the user.
>>
>> I don't see the need to differentiate between premature end and corrupted
>> bytes. Both are problems in another layer anyway and people still stuck
>> using uImage may not like losing extra bytes for an error message anyway.
> 
> I could imagine that knowing that the image is too short would put me on
> another path when searching for the issue.
> 
> Anyway, debugging this shouldn't be too hard, even without this
> information.

Agreed.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] uimage: fix CRC32 verification on NFS

[Sascha Hauer]

On Wed, 02 Oct 2024 18:07:15 +0200, Ahmad Fatoum wrote:
> Reading a file over NFS is prone to return short reads as the file
> content is split over multiple UDP packets and reads won't return
> more than the number of bytes that have gathered in the FIFO.
> 
> The uImage verification code didn't account for this and handled neither
> short reads or the file prematurely ending.
> 
> [...]

Applied, thanks!

[1/1] uimage: fix CRC32 verification on NFS
      https://git.pengutronix.de/cgit/barebox/commit/?id=5676b0b818d8 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>