Various comments - Githubissues

https://github.com/jonpe960/punchboot/blob/master/src/gpt.c#L96

Should not be sizeof(struct gpt_primary_tbl). Replace with sizeof(gpt->backup) for less risk of errors.

https://github.com/jonpe960/punchboot/blob/master/src/gpt.c#L24-L42

len is 64 in the only place this function is used, which means maximum 128 iterations in the loop. That's more than the part->name size of 72. However as long as the name has a null termination it's fine, and I guess that's the case looking at the rest of the code base. It's not really an issue, but I mention it anyway. Also, https://github.com/jonpe960/punchboot/blob/master/src/gpt.c#L37 would write outside the buffer if the maximum number of iterations are reached.

I would define a macro #define membersof(array) (sizeof(array) / sizeof((array)[0])) and repalce this https://github.com/jonpe960/punchboot/blob/master/src/gpt.c#L113 with for (int i = 0; i < membersof(hdr->disk_uuid); i++).

Shouldn't https://github.com/jonpe960/punchboot/blob/master/src/include/pb/gpt.h#L55 use GPT_PART_NAME_MAX_SIZE?

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L437

Maybe use memcmp() instead?

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L449

The fail print will never be executed as the function returns just before this log point.

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L405

This variable is not needed at all, just use err.

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L278

This code doesn't add any value.

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L99-L119

The return value of this function is never used. Could it fail?

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L89-L92 https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L58-L61

Checking for NULL pointer after using it looks a bit weird.

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/caam.c#L55-L83

Maybe call caam_shedule_job_async() and then caam_wait_for_job() instead of duplicating the code?

https://github.com/jonpe960/punchboot/blob/master/src/recovery.c#L211

As I understand it cmd is received over USB. A range check would be appropriate.

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx8m/plat.c#L43-L53

Shouldn't err be checked before accessing f->value, not after?

https://github.com/jonpe960/punchboot/blob/master/src/plat/imx/hab.c#L49

This function should return a bool, not the error code. In fact, if ocotp_read() fails with PB_ERR this function returns true, which is bad as it means the system is secure.

https://github.com/jonpe960/punchboot/blob/master/src/recovery.c#L275

Just do snprintf(version_string, sizeof(version_string), "PB %s",VERSION);.

https://github.com/jonpe960/punchboot/blob/master/src/recovery.c#L539 https://github.com/jonpe960/punchboot/blob/master/src/recovery.c#L180-L195

Is there an upper limit to cmd->size? If not an buffer overflow is possible. Also, recovery_read_data() reads into recovery_cmd_buffer, which is also passed as bfr to the same function. Then a memcpy() is done from recovery_cmd_buffer to bfr, which means the two buffers passed to memcpy() are overlapping, which is not allowed. memmove() must be used instead, or redesign of the code.

https://github.com/jonpe960/punchboot/blob/master/src/recovery.c#L512

Why is the security state read again here, and not only at the beginning of the function?

https://github.com/jonpe960/punchboot/blob/master/src/include/pb/pb.h#L93-L97

Always have parenthesis around parameters where they are used.

Also, the logic is wrong. Test with a=11 and sz=68, __region_start=50 and __region_end 70. The macro says these are not overlapping, when they in fact one of the buffers in contained within the other.

I suggest this instead:

#define PB_CHECK_OVERLAP(__a,__sz,__region_start, __region_end) \
    (((__a) <= ((uintptr_t) (__region_end))) &&                 \
     ((__a) + (__sz) >= ((uintptr_t) (__region_start))))

https://github.com/jonpe960/punchboot/blob/master/src/boot/atf_linux.c#L61

This is dead code.

jonasblixt / punchboot

Various comments #8