Peng,
no, with just the condition 'read < sizeof(*header)', the variable 'read'
(which is a signed integer) will be converted ('balanced') to unsigned to match
the type of 'sizeof()' as per the C standard specification (see
https://stackoverflow.com/a/17294164).
This means that a value read = -2 will be converted to unsigned 0xFFFFFFFE,
the comparison 'read < sizeof(*header)' will be false, and the _spl_load()
function will not return -EIO
In order to fix this bug one has to check first if the value of 'read' is
negative *and* then check 'read' against 'sizeof(*header)'.
To better explain this bug, this morning I wrote the following small C program:
int main()
{
int read = -2;
typedef struct {
int a;
int b;
int c;
} header_t;
header_t hdr;
header_t *header = &hdr;
printf("case 1 - read < sizeof(*header)\n");
if (read < sizeof(*header)) {
printf("would return -EIO\n");
} else {
printf("would continue\n");
}
printf("\n");
printf("case 2 - read < 0 || read < sizeof(*header)\n");
if (read < 0 || read < sizeof(*header)) {
printf("would return -EIO\n");
} else {
printf("would continue\n");
}
printf("\n");
printf("case 3 - read < (ssize_t)sizeof(*header)\n");
if (read < (ssize_t)sizeof(*header)) {
printf("would return -EIO\n");
} else {
printf("would continue\n");
}
printf("\n");
return 0;
}
When I run this program, this is the output:
case 1 - read < sizeof(*header)
would continue
case 2 - read < 0 || read < sizeof(*header)
would return -EIO
case 3 - read < (ssize_t)sizeof(*header)
would return -EIO
Hope this makes sense,
Franco
> On 07/30/2024 9:45 AM EDT Peng Fan <peng....@nxp.com> wrote:
>
>
> > Subject: [PATCH v1] mmc: fix signed vs unsigned compare in read
> > check in _spl_load()
> >
> > Fix signed vs unsigned compare in read check in _spl_load()
> >
> > Issue: when info->read() returns a negative value because of an error,
> > the comparison of 'read' (signed) with 'sizeof(*header)'
> > (unsigned silently converts the negative value into a very
> > large unsigned value and the check on the error condition
> > always return false, i.e. the error is not detected
> > Symptoms: if spl_load_image_fat() is unable to find the file 'uImage',
> > the SPL phase of the boot process just hangs after displaying
> > the following line:
> > Trying to boot from MMC1
> > Fix: first check if 'read' is negative then check its value against
> > 'sizeof(*header)'
> > Reference:
> >
> > Signed-off-by: Franco Venturi <fvent...@comcast.net>
> > ---
> >
> > include/spl_load.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/spl_load.h b/include/spl_load.h index
> > 1c2b296c0a..1e05599d29 100644
> > --- a/include/spl_load.h
> > +++ b/include/spl_load.h
> > @@ -22,7 +22,7 @@ static inline int _spl_load(struct spl_image_info
> > *spl_image,
> >
> > read = info->read(info, offset, ALIGN(sizeof(*header),
> > spl_get_bl_len(info)),
> > header);
> > - if (read < sizeof(*header))
> > + if (read < 0 || read < sizeof(*header))
>
> "read < 0" will imply that "read < sizeof(*header)", so there
> should no need to include this change.
>
> Regards,
> Peng.
>
> > return -EIO;
> >
> > if (image_get_magic(header) == FDT_MAGIC) {
> > --
> > 2.45.2
