Describe the bug

The Bug

When the mcumgr function img_mgmt_read_info() locates a TLV containing the image hash, it will correctly read the hash data into the provided hash data pointer, but will then interpret the hash data as TLV headers on the next loop iteration.

This happens because there are two code-paths for processing TLVs.

One codepath for reading TLVs which are not the image hash:
https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/grp/img_mgmt/src/img_mgmt.c#L326-L330

And another codepath for reading TLVs which are the image hash:
https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/mgmt/mcumgr/grp/img_mgmt/src/img_mgmt.c#L338-L347

In the "not image hash TLV" codepath, the TLV is skipped over by incrementing data_off by the size of the TLV header + the size of the TLV data:

/* Non-hash TLV.  Skip it. */
data_off += sizeof(tlv) + tlv.it_len;
continue;

However, in the "hash TLV" codepath, only the TLV header is skipped over before the loop starts again:

data_off += sizeof(tlv);
if (hash != NULL) {
	if (data_off + IMAGE_SHA_LEN > data_end) {
		return IMG_MGMT_ERR_TLV_INVALID_SIZE;
	}
	rc = img_mgmt_read(image_slot, data_off, hash, IMAGE_SHA_LEN);
	if (rc != 0) {
		return rc;
	}
}
// skalldri: End of loop here

When the loop starts again, it immediately interprets data_off as a TLV header:

while (data_off + sizeof(tlv) <= data_end) {
	rc = img_mgmt_read(image_slot, data_off, &tlv, sizeof(tlv));

At this point, data_off is pointing at the hash data, not a TLV header.

This causes invalid TLV headers to get interpreted by the loop. Thankfully, sizeof(tlv) == 4, and hashes are typically divisible by 4. So once the entire hash has been cast into TLV headers, it naturally ends up pointing at a valid TLV header again.

However, if the hash contains 0xff or 0xffff in the wrong locations, it will trip this check:

if (tlv.it_type == 0xff && tlv.it_len == 0xffff) {
	return IMG_MGMT_ERR_INVALID_TLV;
}

Which can cause random failures to read the image info.

Additionally if the hash contains the bytes IMAGE_TLV_SHA and IMAGE_TLV_SHA_LEN in the wrong locations, it can trip the duplicate hash TLV check:

if (tlv.it_type != IMAGE_TLV_SHA || tlv.it_len != IMAGE_SHA_LEN) {
	/* Non-hash TLV.  Skip it. */
	data_off += sizeof(tlv) + tlv.it_len;
	continue;
}

if (hash_found) {
	/* More than one hash. */
	return IMG_MGMT_ERR_TLV_MULTIPLE_HASHES_FOUND;
}
hash_found = true;

Which will also cause random failures to read image info.

The Fix

Increment data_off after reading the hash so that the next loop iteration does not interpret the hash as TLV headers.

Regression

• This is a regression.

Steps to reproduce

• Create a test MCUboot-compatible image that contains a hash TLV containing all 0xFF's
• Run img_mgmt_read_info()
• Observe the function fails with IMG_MGMT_ERR_INVALID_TLV despite all TLVs being valid

Relevant log output

Impact

Annoyance – Minor irritation; no significant impact on usability or functionality.

Environment

No response

Additional Context

No response