llvm dev - Jul 2017 - 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

2017-07-15 0:10 GMT+08:00 Michal Srb via llvm-dev <llvm-dev at
lists.llvm.org>:
> 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
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


-- 
Wei-Ren Chen (陳韋任)
Homepage: https://people.cs.nctu.edu.tw/~chenwj

On 14 July 2017 at 12:10, Michal Srb via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> 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.
Certainly looks dodgy to me.

Tim.

On Saturday, 15 July 2017 05:46:35 CEST 陳韋任  wrote:
> 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.

Michal

On 07/14/2017 03:18 PM, Tim Northover via llvm-dev
wrote:
> On 14 July 2017 at 12:10, Michal Srb via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> 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.
> Certainly looks dodgy to me.
To me as well. I wonder if we're putting some executable code in a 
section we shouldn't be?

Philip

From: Michal Srb <msrb at suse.com>

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.
---
Multiple people found the EXEC suspicious and nobody knew why would it be
there, so here is patch that removes it.

The one user of this I know of is llvmpipe (Mesa software rasterize), so I
tried rendering my desktop with it and everything worked as usual.

 lib/ExecutionEngine/SectionMemoryManager.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ExecutionEngine/SectionMemoryManager.cpp
b/lib/ExecutionEngine/SectionMemoryManager.cpp
index 8904475f084..41056d0393e 100644
--- a/lib/ExecutionEngine/SectionMemoryManager.cpp
+++ b/lib/ExecutionEngine/SectionMemoryManager.cpp
@@ -139,7 +139,7 @@ bool SectionMemoryManager::finalizeMemory(std::string
*ErrMsg)
 
   // Make read-only data memory read-only.
   ec = applyMemoryGroupPermissions(RODataMem,
-                                   sys::Memory::MF_READ |
sys::Memory::MF_EXEC);
+                                   sys::Memory::MF_READ);
   if (ec) {
     if (ErrMsg) {
       *ErrMsg = ec.message();
-- 
2.12.3