Hi all,

My name is Florin Papa and I work in the Server Languages Optimizations Team at Intel Corporation.

I would like to submit a patch that enables the use of the Intel MPX (Memory Protection Extensions) feature on Python default and 2.7. Invalid memory access problem is commonly found in C/C++ programs and leads to time consuming debugging, program instability and vulnerability. Many attacks exploit software bugs related to invalid memory access caused by buffer overflow/underflow. Existing set of techniques to avoid such memory bugs represent software only solutions which result in poor protection and performance.

Intel MPX introduces new registers and new instructions that operate on these registers. Some of the registers added are bounds registers which store a pointer’s lower bound and upper bound limits. Whenever the pointer is used, the requested reference is checked against the pointer’s associated bounds, thereby preventing out-of-bound memory access (such as buffer overflows and overruns). Out-of-bounds memory references initiate a #BR exception which can then be handled in an appropriate manner.

MPX technology was introduced on the sixth generation of Intel Core processors, code name SkyLake, the successor to Broadwell microarchitecture (only desktop processor is available on the market, server has not been released). For older generations of Intel processors that do not support MPX, nop’s will be added to the instruction stream [1]. 

MPX enabled builds are only possible using gcc 5.1 and newer [2].

To apply the patch please follow these steps:
hg clone https://hg.python.org/cpython
cd cpython
copy mpx_enable.patch to the current directory
hg import --no-commit mpx_enable.patch
./configure –with-mpx
make –j #cores

[1] https://software.intel.com/en-us/articles/introduction-to-intel-memory-protection-extensions
[2] https://gcc.gnu.org/wiki/Intel%20MPX%20support%20in%20the%20GCC%20compiler

Regards,
Florin

In principle adding the option is fine, but do we have a buildbot
with a Skylake processor?  We have lots of options that are untested
and that break periodically.

I think Intel will make sure we do have one.

Florin: how does one determine if a processor supports mxp?  I don't know if the mac mini running the ICC buildbot is new enough to have a Skylake processor, but I suppose there's at least some chance it does.

Why not enablind the option by default if GCC 5.1 or newer is detected? Is there a risk of breaking third-party extensions?

Should we pass the MPX compiler options to third-party extensions by way?

I'm not sure that it's ok to add such change to Python 2.7. It's border line between new feature and supporting a new architecture.

I would suggest to only modify Python 3.6.

Well, since it is arguably security related, I think that pushes it toward the "yes" side.  And yes, I think we could end up "breaking" third party code, as well as python (thus the need for a buildbot), but that may not be a bad thing in this case (that security business again :).

> Why not enablind the option by default if GCC 5.1 or newer is detected? Is there a risk of breaking third-party extensions?
> Should we pass the MPX compiler options to third-party extensions by way?
On a processor that does not support MPX technology, MPX specific instructions will be replaced by nop's and will introduce a performance loss. Also, third party extensions might need to be patched in order to work with MPX. One example is the math module, which needed the bnd_legacy attribute to disable instrumentation when calling libc functions (which are not instrumented and generated compile errors).

> I'm not sure that it's ok to add such change to Python 2.7. It's border line between new feature and supporting a new architecture.
I believe that Python 2.7 should benefit from this change, since it is still used in real life applications, one example being Openstack Swift.

Hi David,

If you are not sure about MPX hardware support, you can find out the processor model with the following commands:

Linux - cat /proc/cpuinfo | grep model
Mac   - sysctl -n machdep.cpu.brand_string

If the processor code is one of the following - i7-6xxx, i7-6xxxT, i5-6xxx, i5-6xxxT, i3-6xxx, i3-6xxxT - it is definitely part of the 6th Generation Intel processors and it supports MPX. Otherwise, it does not.

Whether the hardware is MPX enabled is irrelevant for the build process. GCC 5.1 and higher will generate the MPX specific instructions, which will be turned into nop's at runtime if the processor is not MPX enabled.

> Whether the hardware is MPX enabled is irrelevant for the build process.


The build process is only one part of the equation.  Compilers do have
bugs (Python has been affected by gcc, Visual Studio and suncc bugs),
and we should test the actual generated MPX code.

No go on the ICC mac buildbot then, it's an i7-4578U, so not even gen 5.

This presents the risk of breaking third-party extensions, right?

Wouldn't setting CFLAGS be sufficient?

./configure CFLAGS="-fcheck-pointer-bounds -mmpx"


Otherwise we could also add --with-stack-protector and --with-fortify-source and others. I'm not sure if we should add a special --with*
option for every gcc flag.

Hi Antoine,

Third party modules might need patching to work with MPX, as did the math module, included in this patch. Everything else worked fine in Python 2.7.10+ and 3.6 when running the Grand Unified Python Benchmarks suite and Openstack Swift ssbench benchmark (ssbench supports only 2.7.10+).

Hi Stefan,

When activating the "--with-mpx" flag in ./configure, a check is also performed to see if the compiler is GCC version 5.1 or higher in order to support MPX (and an appropriate error is shown, indicating that the compiler version is wrong). Setting the flags manually will only result in a compiler error.

Also, it might not be obvious for the user from the GCC documentation that both "-fcheck-pointer-bounds" and "-mmpx" flags are needed to enable MPX instrumentation, so adding the "--with-mpx" flag to configure will make it easier to use this feature.

Hi all,

Thank you for your feedback on the code I submitted.

I added the MPX flags to CFLAGS, not CFLAGS_NODIST, because they also need to be included in LDFLAGS.

Please see the new set of patches.

Thank you,
Florin

This looks ok to me as long as it's disabled by default.

> Third party modules might need patching to work with MPX...

If the flags go into CFLAGS, these modules won't compile with
distutils.  Perhaps we should also add LDFLAGS_NODIST, if that
would solve your problem.

Hi all,

I added LDFLAGS_NODIST in order to avoid distutils problems. Please see the updated patches.

Thank you,
Florin

Is there a runtime cost or does the hardware run the bounds checks in parallel with memory accesses?   Are the bounds set when malloc() is called or elsewhere?  I read the provided links but can't say I fully understand how it works exactly (which memory blocks are protected, where the bounds get set, when the registers are loaded, etc).

Also, I'm curious about whether we have direct controls over the bounds.  For example, in a listobject.c or _collections.c object could the bounds be tightened to only include the active data and excluded the unused part of the overallocation?

> Is there a runtime cost or does the hardware run the bounds checks in parallel with memory accesses?   Are the bounds set when malloc() is called or elsewhere?  I read the provided links but can't say I fully understand how it works exactly (which memory blocks are protected, where the bounds get set, when the registers are loaded, etc).

There is a runtime cost associated with MPX instrumentation, which is kept to a minimum due to the use of hardware instructions and registers. When "fcheck-pointer-bounds -mmpx" compilation flags are set, instrumentation is enabled for _all_ memory acceses in the code. This means that when you perform a malloc, the pointer bounds will be set inside the malloc call, and then passed on to your variable. Alternatively, you can manually instrument only regions of interest in your code using GCC functions described here [1].

> Also, I'm curious about whether we have direct controls over the bounds.  For example, in a listobject.c or _collections.c object could the bounds be tightened to only include the active data and excluded the unused part of the overallocation?

Please see __bnd_set_ptr_bounds here [1] for bound manipulation. In order to have a better understanding of what happens when using MPX, I suggest writing a simple C program and look at the assembly code generated (MPX instructions and registers begin with bnd). You can use the following steps:

gcc -g -c test.c -O2 -fcheck-pointer-bounds -mmpx
objdump -d -M intel -S test.o


[1] https://gcc.gnu.org/wiki/Intel%20MPX%20support%20in%20the%20GCC%20compiler#Compiler_intrinsics_and_attributes

> This means that when you perform a malloc, the pointer bounds will be set inside the malloc call, and then passed on to your variable.

For this to be useful in Python, you would have to annotate Python's small object allocator to designate it as a malloc()-like function, no?

No modifications need to be made to the small object allocator. The malloc situation was just an example, it does not mean that setting pointer bounds occurs only in a malloc call. It also occurs when you declare a static array or when you initialize a new pointer:

int *x = (int*)malloc(10 * sizeof(int));
int *y = x; // the bounds for x will be passed on to y

When using "fcheck-pointer-bounds -mmpx" _all_ memory accesses will be instrumented.

The small object allocator uses large mmap-allocated arenas of 256 KB (IIRC) and carves small objects out of it.  So unless the pointer returned by PyObject_Malloc() has its bounds set manually using one of the intrinsic functions (*), MPX would believe the bounds of small objects are the bounds of the 256 KB arenas containing them, right?

(*) I'm assuming __bnd_set_ptr_bounds()

FYI In the discussion of the PEP 445 "Add new APIs to customize Python
memory allocators" it was proposed to add runtime option to choose the
memory allocator. At least for debug purpose, it would help to be able
to use malloc() for *all* Python memory allocations. Would it help to
find more bugs with MPX?

It's not possible to disable our fast allocator for small objects,
disabling it has a high cost on performances (Python would be much
slower).

Hi Antoine,

I understand the problem with the small object allocator now. I will have a closer look at it and come back with a solution. Thank you for pointing this out.

I have modified the small object allocator to set proper bounds  for the pointers it returns. Please see the updated patches.

Thanks. For the record, the whole test suite still passes? This is good to know :)

I ran the full Grand Unified Python Benchmarks suite on Python 3.6 and 2.7 before submitting the patches and everything went well. Sorry I did not mention it :)

I meant the test suite ("python -m test.regrtest"). I presume you did, just checking :-)

I ran regrtest now and there are a few tests that fail. I will look into the cause of this and provide a solution.

Hi all,

I updated the patch after fixing crashes that occurred at compile and run time on Ubuntu 15.10, with kernel version 4.2.0-16-generic, LD version 2.25.1 and GCC version 5.2.1. The Python MPX binary will be built with –O0 because other optimization levels will change instruction order and cause crashes, making it useful for debugging purposes. The minimum GCC version was upgraded to 5.2 for the MPX build in order to make sure there is full support for this feature.

All tests from regrtest pass, except pest_pyclbr which also fails on a non-MPX build and two other tests, test_capi and test_faulthandler, which fail because the output expected after a segmentation fault is generated is modified by MPX (which reacts to a memory violation).

The built MPX Python binary was evaluated (“./python -m test.regrtest”) on two older Intel generation processors, Haswell and Broadwell, and no functional issue was observed.  However, we don’t know its impact on third-party extensions.

Thank you,
Florin

> The Python MPX binary will be built with –O0 because other optimization levels will change instruction order and cause crashes, making it useful for debugging purposes.

I've trouble understanding this: Is the gcc mpx feature not stable?



Looking at the patch, we'd introduce many new macros for everyone
to learn, but I'm generally not too enthusiastic about the proliferation
of memory protection schemes after having been bitten by a bug in
FORTIFY_SOURCE memmove wrappers. :(

Hi Stefan,

The GCC MPX feature proved stable when using O0, having tested multiple times with regrtest on Skylake, Broadwell and Haswell.

Still, this is a new feature and GCC might improve it with time.

Florin, thanks for the explanation.  I'd suggest to wait until
the mpx support is stable in gcc.

Some security features in gcc never take off (-ftrapv is broken,
libmudflap is deprecated, ...) so it would be nice to have some
reassurance.

Please see the updated patch after Zachary Ware's review. Thank you for the feedback.

[ Stefan Krah]
> I'd suggest to wait until the mpx support is stable in gcc.

I concur.

Hi all,

We have set up a Skylake buildbot, which could be used for MPX builds. The errors that appear when running regrtest are proxy related and we are looking into a solution for that. There is no problem for tests that do not access network resources.

The buildbot is available here:
http://buildbot.python.org/all/builders/x86-64%20Ubuntu%2015.10%20Skylake%20CPU%202.7

Thank you,
Florin Papa

Hi,

I've reviewed the latest patch.
I have a more general question: the patch sprinkles some Py_INIT_BOUNDS() calls in various places. What the macro does is set the pointer's bounds to "INIT", which basically means disable any checks (the pointer is allowed to access all memory). Is there a reason for that? MPX checks for out-of-bounds accesses, so setting pointer bounds to "INIT" seems to defeat the point.

(if MPX otherwise complains about out-of-bounds accesses in CPython, perhaps they are genuine bugs that deserve fixing, or perhaps the annotations in the patch are not good enough)

I'm wary of making such extensive changes throughout the codebase for an optional processor-specific feature of limited benefit and unproven worth.   

With each new macro and trick (Py_VARIABLE_SIZE and Py_INIT_BOUNDS), we make it harder for people to read, create, and maintain our code.  The barrier to entry is getting too high IMO.

If this were just a makefile change, it would be different.  But it touches a lot of code in ways that will be unfamiliar looking to most C programmers:

  Py_ssize_t ob_array[1] Py_VARIABLE_SIZE;   /* Looks weird and confusing */

A surprising number of files are being modified:
--- a/Include/asdl.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/bytesobject.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/frameobject.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/longintrepr.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/memoryobject.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/object.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/objimpl.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/pyport.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Include/tupleobject.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Makefile.pre.in	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/_ctypes/ctypes.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/_tracemalloc.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/expat/xmlparse.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/mathmodule.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/sre.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/sre_lib.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Modules/winreparse.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/dict-common.h	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/dictobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/listobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/longobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/memoryobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/object.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/obmalloc.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/typeobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Objects/unicodeobject.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Parser/grammar.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Python/ast.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Python/dtoa.c	Mon Nov 02 09:17:08 2015 -0800
--- a/Python/pyhash.c	Mon Nov 02 09:17:08 2015 -0800
--- a/configure	Mon Nov 02 09:17:08 2015 -0800
--- a/configure.ac	Mon Nov 02 09:17:08 2015 -0800
--- a/pyconfig.h.in	Mon Nov 02 09:17:08 2015 -0800

Le 25/11/2015 03:23, Raymond Hettinger a écrit :
> 
> I'm wary of making such extensive changes throughout the codebase
> for
> an optional processor-specific feature of limited benefit and unproven
> worth.

This is indeed non-trivial. What I'm willing to known is if it helps
detect unknown bugs and vulnerabilities in the code base (for example
when running the test suite or sample workloads).

The Py_VARIABLE_SIZE macro is trivial to understand, and is actually a
useful annotation to the code (a kind of executable documentation). The
BOUNDS macros are a bit more interesting, but their semantics look
simple if you don't want to think too much about the underlying HW
implementation :-)

> Py_ssize_t ob_array[1] Py_VARIABLE_SIZE;   /* Looks weird and confusing */

Yes, I dislike that, too.  The question is why gcc has supported
the "struct hack" for more than 20 years but needs an annotation
just for MPX.

Is the annotation also needed for the C99 version (ob_array[])?


Which brings me again to the topic that the MPX support needs to
stabilize in gcc. ;)

Hi Antoine,
 
The Py_INIT_BOUNDS calls were used because MPX generated a very large number of error messages about pointer bounds violations at compile or run time, that made Python unusable. The approach was to analyze the errors and ignore checking if no obvious violation took place, in the hope to find the root cause.
 
The problem is that the errors can come from an actual bug or from a false positive that can be propagated throughout the code. While we could not find evidence of actual bugs, we found 2 examples of the latter, in listobject.c (line 1100, in the binarysort function) and dictobject.c (line 1797 in dict_keys function and line 1892 in dict_items).
 
The problem is caused by this coding pattern that is used in the two instances mentioned above: a pointer to an allocated memory zone is used to access a different allocated memory zone by adding the difference between their start addresses to that pointer. Although this newly formed address is valid, in the context of the pointer used for this operation it is outside its bounds so it’s signaled as a bounds violation.
 
p *----------------*                      q *-------------------------------*
   <-------------------------------------->
                     offset

p and q point to valid memory zones, being separated by an offset. If we do something like 
               new_pointer = q – offset; // which is actually equal to p
               value = *new_pointer;      // dereferencing will generate an MPX bounds violation, because
                                          // new_pointer will keep q’s original bounds
 
I will rewrite the code for these cases and provide the new patch as soon as I have it.

Regards,
Florin

Hi Stefan,

> Yes, I dislike that, too.  The question is why gcc has supported
> the "struct hack" for more than 20 years but needs an annotation
> just for MPX.
The "struct hack" represents a problem for MPX because it intentionally exceeds the bounds of the array declared inside in order to support a variable sized structure. Without the Py_VARIABLE_SIZE annotation, MPX will generate a bounds violation when accessing outside the declared size of the array.

> Is the annotation also needed for the C99 version (ob_array[])?
I tested and for the C99 version the annotation is not needed. Apparently ob_array inherits the bounds of the structure in this case.

Regards,
Florin

Hi all,

Our latest effort on enabling MPX in CPython has concentrated around eliminating all INIT_BOUNDS and BND_LEGACY attributes that are used to bypass bounds checking for certain pointers. In order to avoid using these attributes, we needed to find and fix the root cause of the problems. The main issue was represented by the small object allocator (Objects/obmalloc.c), which was performing some operations that MPX considered unsafe (pointer jumping). A similar problem was found in the allocator used by the garbage collection module (Modules/gcmodule.c). These issues, as well as other minor operations considered unsafe by MPX (Objects/listobject.c, Objects/dictobject.c) have been addressed so far.

As a result, we were able to eliminate all INIT_BOUNDS and BND_LEGACY attributes from the code. Also, we identified the optimization flag that caused crashes when compiling with –O3, which is –fipa-icf. Compiling with “-O3 –fno-ipa-icf” now works fine. The entire regrtest suite passes, except test_capi and test_faulthandler. Test_capi fails because of a “\n” outputted by the MPX runtime at stdout instead of stderr (fixed in the GCC 6 trunk). Test_faulthandler fails because we have disabled the faulthandler module when MPX is active, as it produced crashes since both the faulthandler and the MPX runtime overwrite the default SIGSEGV handler and the new handlers would interfere with each other.

The current patch works on GCC 5.3.0, which solves a linking problem with libmpx, present in GCC 5.2.1. We still have some problems, such as bounds warnings that only appear once in 10 runs for a few of the tests, but do not cause crashes or failed tests. The biggest problem we face is the presence of pointers that do not have bounds. These could be the result of some bugs we found in MPX:

    1. Calling strlen and memset (possibly others) for the first time in a program will not be subject to MPX checks
    2.Copying an array of pointers to a new location will reset the first pointer’s bounds (deep copy of the pointer bounds fails)
The first problem was solved by upgrading ldd to version 2.22, while the second issue will be solved by the GCC 6 release (around April 2016), which will offer more stable support for MPX.

Therefore, we have decided to wait until the GCC 6 release to provide a final version of the MPX patch for CPython. Meanwhile, you can see the latest modifications we have made in the patch attached.

Thank you,
Florin Papa

For the record, the llvm-dev is currently having a discussion thread about MPX. Some of the feedback is a bit critical:
http://lists.llvm.org/pipermail/llvm-dev/2016-February/094828.html

Also this quite comprehensive report:
https://github.com/google/sanitizers/wiki/AddressSanitizerIntelMemoryProtectionExtensions

Let's have another look at this enhancement for 3.7. Hopefully we have some machines to develop with and test MPX, too. I don't have any machine at home that supports hardware MPX. Does any of our buildbots have a Skylake with MPX?

Hi Christian,

There is a Skylake buildbot that has MPX capabilities. Please find it here:

http://buildbot.python.org/all/builders/x86-64%20Ubuntu%2015.10%20Skylake%20CPU%203.6

Regards,
Florin

I'm going to formally reject this, since GCC removed -fmpx support.

> I'm going to formally reject this, since GCC removed -fmpx support.

"The MPX extensions to the C and C++ languages have been deprecated and will be removed in a future release."
https://gcc.gnu.org/gcc-8/changes.html

Oh. It seems like the reason is to "reduce the maintenance burden".

Some links:

* https://www.kernel.org/doc/Documentation/x86/intel_mpx.txt
* https://en.wikipedia.org/wiki/Intel_MPX
* https://www.phoronix.com/scan.php?page=news_item&px=GCC-Possible-MPX-Deprecation

A patch has been proposed to remove MPX from GCC 9 (no merged?):
https://www.phoronix.com/scan.php?page=news_item&px=GCC-Patch-To-Drop-MPX