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

[Sascha Hauer <s.hauer@pengutronix.de>]

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 |