esp_himem_map() calculates wrong pointer if range_offset is nonzero (IDFGH-3713)

[devanlai]

Environment

• Development Kit: none
• Module or chip used: ESP32-WROVER-E
• IDF version: v4.3-dev-472-gcf056a7d0
• Build System: idf.py
• Compiler version: xtensa-esp32-elf-gcc (crosstool-NG esp-2020r2) 8.2.0
• Operating System: Windows
• (Windows only) environment type: Plain Command Prompt.
• Using an IDE?: VSCode with official Espressif IDF extension
• Power Supply: USB

Problem Description

The esp_himem_map() function accepts a range_offset parameter that selects the virtual memory range within an existing range handle to use. For any value of range_offset other than zero, the resulting pointer returned via *out_ptr is calculated incorrectly.

By simple inspection of line 338 in esp_himem.c:

//Set out pointer
*out_ptr = (void *)(VIRT_HIMEM_RANGE_START + (range->block_start + range_offset) * CACHE_BLOCKSIZE);

range_offset is in bytes, but it's being added to range->block_start which is counting cache blocks and then being multiplied by CACHE_BLOCKSIZE.

Instead, it should be calculated one of two possible more-or-less equivalent ways:

// Using range_block, which is calculated earlier as range_block = range_offset / CACHE_BLOCKSIZE
*out_ptr = (void *)(VIRT_HIMEM_RANGE_START + (range->block_start + range_block) * CACHE_BLOCKSIZE);

or:

 // Using range_offset directly
 *out_ptr = (void *)(VIRT_HIMEM_RANGE_START + (range->block_start * CACHE_BLOCKSIZE) + range_offset);

[Alvin1Zhang]

Thanks for reporting, we will look into.

[AxelLin]

Thanks for reporting, we will look into.

I think this bug report is valid.
@devanlai already provided the fix in the bug report, I'm wondering why nobody fix it?

[AxelLin]

Thanks for reporting, we will look into.

Any progress?

[AxelLin]

Thanks for reporting, we will look into.

@Alvin1Zhang
This was reported by 2020 and it's 2022 now, I'm wondering if anyone will take a look.

[alyf80]

Just hit this. Any chance it will ever be fixed?

[AxelLin]

Just hit this. Any chance it will ever be fixed?

Does the change mentioned in #5639 (comment) work for you?

[alyf80]

@AxelLin It does.

FWIW, current release/v4.4, release/v5.0, release/v5.1 and master are also affected.

[alyf80]

@ginkgm

It's not a matter of API vs. implementation: the actual mapping works fine and just in the way described in the documentation.

The problem is a bug in the computation of out_ptr, which instead of being a valid pointer to the beginning of the mapped block inside the range ends up with a totally incorrect value whenever the function is called with a non-zero range_offset.

On the positive side, the bug cannot go unnoticed because the incorrect pointer is always beyond the end of the valid ESP32 memory address range, so attempting to dereference it immediately causes an abort.

[ginkgm]

@alyf80 ,

I see. I have reconsidered this and think your proposal make sense.

No one can call the function with range_offset by block num but work well. So it shouldn't be treated as breaking change.

The fix is on the way.

[AxelLin]

@ginkgm v5.0 branch still needs this fix.