SectionMemoryManager::finalizeMemory ... read only data become executable?

Hi,

I am studying the llvm code and I noticed something that looks like a bug. If
it is intentional for some reason, it is not obvious why.

In lib/ExecutionEngine/SectionMemoryManager.cpp in
SectionMemoryManager::finalizeMemory method:

// Make code memory executable.
ec = applyMemoryGroupPermissions(CodeMem,
                                 sys::Memory::MF_READ | sys::Memory::MF_EXEC);

...

// Make read-only data memory read-only.
ec = applyMemoryGroupPermissions(RODataMem,
                                 sys::Memory::MF_READ | sys::Memory::MF_EXEC);

                                                        ^^^^^^^^^^^^^^^^^^^^^
                                                     Why is this also MF_EXEC?

Best regards,
Michal Srb

SectionMemoryManager allocates executable memory section for MCJIT
(JIT emit code then execute).
So the bottomline is making the section sys::Memory::MF_EXEC. As for
RODataMem, we also need
to make sure it's read-only. You can compare it to RWDataMem, which
has write permission.

Regards,
chenwj

Certainly looks dodgy to me.

Tim.

That makes sense. My question is why is RODataMem made READ and EXEC? It
sounds like it should be only READ.

Michal

Hi Michal,

SectionMemoryManager allocates executable memory section for MCJIT
(JIT emit code then execute).
So the bottomline is making the section sys::Memory::MF_EXEC. As for
RODataMem, we also need
to make sure it's read-only. You can compare it to RWDataMem, which
has write permission.

That makes sense. My question is why is RODataMem made READ and EXEC? It
sounds like it should be only READ.

I guess my reply is a little bit misleading. Since JIT will emit and
*execute* code in the memory,
the memory should be set as MF_EXEC at least. As RODataMem is
read-only, we should make
it MF_READ as well. I think MF_EXEC and MF_READ will be set after the
code is emitted.

Regards,
chenwj

Not all bytes that get emitted are intended to be executed. In ELF
section terms, .text needs to be executable, but .rodata doesn't. It
contains data that will be loaded by the actual instructions so it
needs to be readable, but making it executable is a potential security
risk.

Cheers.

Tim.

+ Lang. :slight_smile:

To me as well. I wonder if we're putting some executable code in a section we shouldn't be?

Philip

There doesn't seem to be a reason why would RODataMem need to be executable.
It looks like the flags were accidentally coppied from few line above.