[PATCH v1] mmc: fix signed vs unsigned compare in read check in _spl_load()

[Franco VENTURI]

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.fan at 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 <fventuri at 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