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@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@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

Sean, thanks for the quick response.

I just resubmitted version 2 of that patch with a cast to int.

Franco

On 07/31/2024 8:52 AM EDT Sean Anderson seanga2@gmail.com wrote:

Hi Franco,

On 7/30/24 09:30, Franco Venturi wrote:

...

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: https://stackoverflow.com/questions/17293749/sizeof-operator-in-if-statement

Signed-off-by: Franco Venturi fventuri@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)) return -EIO;

  if (image_get_magic(header) == FDT_MAGIC) {

Since read and int and sizeof(*header) < INT_MAX, I think the best solution is to cast the latter to an int.

--Sean