[TLS / hw acceleration] AES ECB for NUCLEO_F439ZI #3691

adustm · 2017-02-03T14:25:33Z

Description

This pull request contains the very first HW acceleration for STM devices
Could you review it and let us know if it fits the requested process for HW acceleration in mbedtls feature ?
I've also added aes in TESTS/mbedtls/selftest/main.cpp
+---------------------------+--------+--------+--------+
| test case | passed | failed | result |
+---------------------------+--------+--------+--------+
| mbedtls_aes_self_test | 1 | 0 | OK |
| mbedtls_entropy_self_test | 1 | 0 | OK |
| mbedtls_sha256_self_test | 1 | 0 | OK |
| mbedtls_sha512_self_test | 1 | 0 | OK |
+---------------------------+--------+--------+--------+

Once we agree on this files format and structure, we will be able to add soon AES-CBC mode, SHA1 etc...

Status

READY

Steps to test or reproduce

mbed test -m NUCLEO_F439ZI -t IAR -n tests-mbedtls-selftest

cc @bcostm @screamerbg @sbutcher-arm @0xc0170

jeromecoutant · 2017-02-03T15:12:13Z

Hi
Is it OK to add some TARGET_xxx files in the features directory ?
Could we move these files into targets directory ?

adustm · 2017-02-06T09:30:44Z

Hi @jeromecoutant
It would be nice but I don't know the impact on code size etc...

@0xc0170 ?
Jerome and I would like to move mbedtls/target_STM code to the usual mbed-os/targets/... TARGET_STM directory.
Do you know if we can do that or not ? What is the impact of doing this ?

Kind regards
Armelle

0xc0170 · 2017-02-06T10:04:12Z

Jerome and I would like to move mbedtls/target_STM code to the usual mbed-os/targets/... TARGET_STM directory.
Do you know if we can do that or not ? What is the impact of doing this ?

@sbutcher-arm Please review this PR.

screamerbg · 2017-02-09T10:34:13Z

CC @RonEld

adustm · 2017-02-14T13:14:53Z

Hello @RonEld @sbutcher-arm
Could one of you review my PR please?
Could you also let me know if we must keep STM's files in the feature directory or if we can move them to the targets usual STM directory ?

Cheers
Armelle

simonbutcher · 2017-02-14T17:00:13Z

Hi @adustm.

The idea of keeping the mbed TLS acceleration driver code in the features/mbedtls/targets directory is to separate out the code that uses the mbed HAL from the code that uses the mbed TLS interfaces, as well as making it clearer that code that is pushed to the HAL targets directory has a different approvals process from that pushed to the mbed TLS targets directory, as the cryptographic code needs an additional security review.

Does this present any issues for you?

We're working on reviewing this code, and we need feedback and sign-off from myself, @yanesca and @andresag01 before we can approve this PR.

adustm · 2017-02-14T17:38:13Z

Hi @sbutcher-arm
Thank you for your comment, I'll wait for the review results.

to separate out the code that uses the mbed HAL from the code that uses the mbed TLS interfaces

features/mbedtls/targets/TARGET_STM/aes_alt.c is calling functions from targets/.../TARGET_STM32F4/device/stm32f4_hal_cryp.c

so from ST's point of view, ST code is separated out

Does this present any issues for you?

Let me try to explain the reason for this request from my colleagues and I. If we can do it, let's go, if we can't, let it go. It is ARM to decide at the end :-)

ST code is spread out in several features (mbedTLS / usb / lwip... ).
As you may know we are providing support for more than 35 platforms.
There are people working in parallel (from ST or not), not knowing into deep details the work of others.

ST code is specific for our targets:

we are handling the pins and call to hw functionalities for I2C / SPI / UART / etc... in targets directory
We are handling the pins and ST HW for the Ethernet phy in lwip feature directory + it calls ST HAL functions from targets directory
We are handling the call to hw acceleration in mbedtls feature directory + it calls ST HAL functions from targets directory
we are handling USB call in unsupported/USB directories
And eventually we are handling the call to hw entropy in targets directory

When every code is located in a single directory tree we maximize the chance that every ST contributor is aware of the changes coming from others, and is better aware of the impact of his/her changes.

mbed compilation and link mechanism is creating a long list of include directories, so the code could be located here or there, I think it has no impact, has it ?

Anyway I have written this pull request following the specification (ST code in features/mbedtls).
The whole discussion is coming from an internal remark we had between ST people. We can now decide to take it into account or classify the discussion.

Kind regards
Armelle
cc @jeromecoutant @LMESTM @bcostm @jamike

simonbutcher · 2017-02-14T18:24:46Z

Hi @adustm,

I think this is a useful discussion, and we have been seeking feedback from partners on the hardware acceleration interfaces. However, this is going outside the scope of this PR, and it may be more useful to discuss this directly.

To clarify - the targets directory is concerned with the mbed HAL and the interfaces mbed OS presents. mbed TLS has a life outside of mbed OS and is used on other platforms, and has separate and distinct interfaces itself. For instance, note mbed OS has a TRNG HAL API, whereas mbed TLS has it's own entropy interface, a mbed OS platform code brings the two interfaces together.

I think it's understandable that platform/SoC/driver code that might reside in the TARGETS directory may be called by driver code in the mbed TLS code driving the hardware acceleration code. I don't think that is an issue, and you are correct to say we could place the sources in other places (and indeed could move them later). This is really about logical organisation and management.

andresag01

I have done an initial review and found the following:

There are some issues with the use of the _ALT macros.
I do not think this code is thread safe.
I think there is an opportunity to reconcile the mbedtls_aes_context structure with CRYP_HandleTypeDef as some of the data is duplicated, unused or redundant.
Pointed out places where the function return codes are not checked for errors.

andresag01 · 2017-02-15T11:28:21Z

features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h

+#define MBEDTLS_AES_SETKEY_ENC_ALT
+#define MBEDTLS_AES_SETKEY_DEC_ALT
+#define MBEDTLS_AES_ENCRYPT_ALT
+#define MBEDTLS_AES_DECRYPT_ALT


The AES module supports replacing the full module by defining the macro MBEDTLS_AES_ALT or replacing functions by defining the macros MBEDTLS_AES_SETKEY_ENC_ALT, MBEDTLS_AES_ENCRYPT_ALT, etc... However, it is my understanding that when MBEDTLS_AES_ALT is defined, the function specific replacement macros become irrelevant, so I do not think it makes sense to define these here.

I think that this file should contain the macro definition for MBEDTLS_AES_ALT instead of the targets.json file.

Ok I will try that

andresag01 · 2017-02-15T11:35:08Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+ */
+#if defined(MBEDTLS_AES_ALT)
+
+#include <stdio.h>


Because this is mbed TLS related code, I would advise to include the mbed TLS platform abstraction header (i.e. mbedtls/platform.h) instead of stdio.h, unless there is good reason to do this.

I had a look at all the code in this file and it does not look like there are references to any symbols from stdio.h. Could you please remove this include, or explain why it is required?

The code in this file uses a few macros such as MBEDTLS_CIPHER_MODE_CTR which are defined in mbedtls/config.h, but the file is not included here.

thanks I will check these

andresag01 · 2017-02-15T11:37:37Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+#include <stdio.h>
+#include "cmsis.h"
+#include "string.h"
+#include "aes.h"


I think this is including the mbed TLS header file for the AES module. If this is the case, I would encourage to modify the include to #include "mbedtls/aes.h" to avoid any aliasing problems.

andresag01 · 2017-02-15T11:40:42Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+ * \brief          Initialize AES context
+ *
+ * \param ctx      AES context to be initialized
+ */


Style: Normally, the doxygen-style documentation for each module should be written in the header file instead of the .c file. So, I would advise to remove these doxygen comment blocks. Furthermore, the aes_alt.h file already contains the same comment blocks.

andresag01 · 2017-02-15T11:56:08Z

targets/targets.json

@@ -899,7 +899,7 @@
        "extra_labels": ["STM", "STM32F4", "STM32F439", "STM32F439ZI", "STM32F439xx", "F429_F439"],
        "supported_toolchains": ["ARM", "uARM", "GCC_ARM", "IAR"],
        "progen": {"target": "nucleo-f439zi"},
-        "macros": ["TRANSACTION_QUEUE_SIZE_SPI=2"],
+        "macros": ["TRANSACTION_QUEUE_SIZE_SPI=2", "MBEDTLS_CONFIG_HW_SUPPORT", "MBEDTLS_AES_ALT"],


I think the targets.json should not contain defines such as MBEDTLS_AES_ALT. Instead, these defines are expected to be in the mbedtls_device.h file.

I will try that

Thank you very much @andresag01 for this detailled and interesting review :+1
I will rework that and send updates

andresag01 · 2017-02-15T14:08:30Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+        return(MBEDTLS_ERR_AES_INVALID_KEY_LENGTH);
+
+    /*------------------ AES Decryption ------------------*/
+    if(mode == MBEDTLS_AES_DECRYPT) /* AES decryption */


This comment is a little redundant...

andresag01 · 2017-02-15T14:08:45Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+        mbedtls_aes_decrypt( ctx, input, output );
+    }
+    /*------------------ AES Encryption ------------------*/
+    else /* AES encryption */


This comment is a little redundant

andresag01 · 2017-02-15T14:10:57Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+        // unaccelerated mode
+        //
+    }
+#endif


I think this block of code is only for x86 machines. Please consider removing it...

andresag01 · 2017-02-15T14:16:42Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+                          unsigned char output[16] )
+{
+
+    HAL_CRYP_AESECB_Encrypt(&hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10);


I think it would be good to check this function's return code for errors.

it does return timeout for instance. but mbedtls_aes_encrypt returns void. How do you usually handle it ?

I think we have an issue here. The interface was originally designed for a software implementation of AES and was later re-purposed for other implementations.

Here, I don't think the interface suits what we're trying to achieve in hardware, therefore I think we should deprecate this interface, and implement a new interface which enables a error return code.

@andresag01 - Can you raise an issue/enhancement on the mbed TLS github repo?

@adustm - I think we can accept this code as it is at the moment, but we'll probably want to refine this code to work with a new interface later. In the meantime, can we remove the mbedtls_printf() and replace it with a comment?

@sbutcher-arm: I have raised the new issue in the mbed TLS repository at Mbed-TLS/mbedtls#817

andresag01 · 2017-02-15T14:16:49Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+                          unsigned char output[16] )
+{
+
+    HAL_CRYP_AESECB_Decrypt(&hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10);


I think it would be good to check this function's return code for errors.

same comment

screamerbg · 2017-02-16T13:36:00Z

@adustm @sbutcher-arm I think there are 2 points of views:

how features in mbed OS are organized
how partner ports for targets in mbed OS are organized

I agree with @adustm about having all target support code in one place under targets. The ST team is supporting significant amount of targets and having this separation is inconvenient, makes it harder for the mbed Partner teams to maintain (it's all over the place already) and reduces the chance for community contributions.

@sbutcher-arm wrote:
mbed TLS has a life outside of mbed OS and is used on other platforms, and has separate and distinct interfaces itself.

This PR is not to the mbed TLS specifically (e.g. this is not http://github.com/ARMmbed/mbedtls), but to extend an existing target support in mbed OS for TLS features. If the TLS team would like to request from ST to contribute this HW crypto support to http://github.com/ARMmbed/mbedtls then please follow the formal process via email.

Also if there is visibility issue, then this could be addressed by adding a label for the ARM mbed TLS team to review.

Perhaps this is a wider question (hence cc @sg-):

where do features live - currently in /features
where do drivers code live - currently in /targets for some, but /features for other
where do glue layers live - currently /targets for HAL, LWIP /features/FEATURE_LWIP

Also it's important to understand the maintainability point of view, e.g. how much code mbed Partner teams are managing.

For example, the following structure may satisfy both ST team code maintenance and glue layers for features support

/targets
 -> TARGET_<vendor>
 ---> TARGET_<mcu_family>
-------> FEATURE_LWIP
-----------> all LWIP driver code and glue layer for that MCU family

With this structure the feature driver code and the feature glue layer for would be gated by the presence of the feature (meaning that it won't be compiled if the feature is not enabled). Also it would be in one place from mbed Partner maintenance point of view.

sg- · 2017-02-16T15:22:19Z

Top level target code is organized to contain MCU SDKs and a binding to the mbed HAL. Any feature integrated as a mbed HAL interface and enabled by device_has should remain in feature/xxx/TARGET_xxx At the point a mbed HAL interface and tests come available for a features integration the implementation should move into the top level targets directory.

In this case, I agree with @sbutcher-arm

The idea of keeping the mbed TLS acceleration driver code in the features/mbedtls/targets directory is to separate out the code that uses the mbed HAL from the code that uses the mbed TLS interfaces, as well as making it clearer that code that is pushed to the HAL targets directory has a different approvals process from that pushed to the mbed TLS targets directory, as the cryptographic code needs an additional security review.

From the developer guide for hardware acceleration: https://docs.mbed.com/docs/mbed-os-handbook/en/latest/advanced/tls_hardware_acceleration/#how-to-implement-the-functions

Clone the mbed OS repository and copy the source code of your function definitions to the features/mbedtls/targets/TARGET_XXXX directory specific to your target. Create a pull request when your code is finished and production ready. You may create a directory structure similar to the one you have for the HAL if you feel it appropriate.

adustm · 2017-02-16T15:54:44Z

Hello,
Thank you @screamerbg and @sg- for your answers and comments.
I think the discussion can be closed.

I'll start working on the PR review comments.
Kind regards

adustm · 2017-02-17T17:02:13Z

Hi @andresag01
I've reworked the code following your recommendations.
MBEDTLSxxx_ALT tags should be well handled
I've added AES_CBC hw acceleration
AND : I've fixed the 'not thread safe' issue you pointed out (and now I have CTR-DRBG ok)

I hope everything will be fine now.
Kind regards
Armelle

0xc0170 · 2017-02-21T11:41:14Z

Another round of review please?

retest uvisor

adustm · 2017-02-23T07:41:56Z

Hello, is there anything blocking ?

0xc0170 · 2017-02-23T10:06:55Z

There's a conflict to be resolved

@andresag01 @sbutcher-arm please review

andresag01

@adustm: Thanks for taking into account the previous review comments. I have reviewed the code once more and most of the issues I pointed out seem to have been resolved.

From my point of view, there are still a couple of issues left:

The functions mbedtls_aes_encrypt() and mbedtls_aes_decrypt() do nor have return values, making difficult to check whether the crypto accelerator succeeded. This touches on the API design, so I have raised the issue mbedtls_aes_encrypt() and mbedtls_aes_derypt() do no have return values Mbed-TLS/mbedtls#817.
There have been some improvements, but I do not think that this code is thread-safe. Normally, we achieve this in mbed TLS by using the threading API (threading.h). Please note that there is a PR to add support for this: mbed TLS: threading support - DRAFT - Do not merge #3150

RonEld · 2017-02-23T13:30:01Z

I have looked in the changes, and I saw that there is many code duplicity, and copy\paste from the sw implementation. I beleive this is due to the current implementation of the HW configuration. This code duplication could have been resolved by MBEDTLS_AES_SETKEY_ENC_ALT , MBEDTLS_AES_SETKEY_DEC_ALT , MBEDTLS_AES_ENCRYPT_ALT , MBEDTLS_AES_DECRYPT_ALT , but the mbedtls_aes_context needs to be modified to support the HW AES, and this is currently not possible without MBEDTLS_AES_ALT.
According to that, I opened a github enhancement request , that I believe could remove this duplicity.
Other than that, the changes look good to me

simonbutcher

Requires minor changes, but otherwise approved by me.

simonbutcher · 2017-02-28T11:05:55Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+                          unsigned char output[16] )
+{
+
+    if (HAL_CRYP_AESECB_Encrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10) !=0)


We've identified that we need to change this function so it can return an error code, but until we do, I don't think we should be calling mbedtls_printf() here, unless it's restricted to be in just debug builds or similar.

hello @sbutcher-arm
Do you suggest that I remove the mbedtls_printf and replace it by a comment line
/* return error code once mbedtls_aes_encrypt and mbedtls_aes_decrypt functions will return one */
?

simonbutcher · 2017-02-28T11:06:16Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+{
+
+    if(HAL_CRYP_AESECB_Decrypt(&ctx->hcryp_aes, (uint8_t *)input, 16, (uint8_t *)output, 10))
+        mbedtls_printf( "HAL_CRYP_AESECB_Decrypt timeout\n" );


Same point as above on mbedtls_printf().

same question as above

simonbutcher · 2017-02-28T11:10:06Z

Hi @adustm,

Looking over the code, and based on the feedback from @andresag01 and @RonEld, I can see we need to refine the hardware acceleration interface that mbed TLS presents. I don't want those changes to hold up this work, so we'll do that independently and then later can revise this code to take advantage of those changes. Please feel free to provide more feedback on those changes.

Thanks for your patience as we've worked through this pull request.

My only concern is the calls to mbedtls_printf() in case of an error, but that more shows a problem in our interfaces than your code.

adustm · 2017-02-28T13:00:54Z

Hello @RonEld
Your comment about thread safety is not clear to me and the PR you're pointing at is not merged.
Do you have a document, or a code example that I could read to understand better ?

Thank you to the 3 reviewers
Kind regards

+---------------------------+--------+--------+--------+ | test case | passed | failed | result | +---------------------------+--------+--------+--------+ | mbedtls_aes_self_test | 1 | 0 | OK | | mbedtls_entropy_self_test | 1 | 0 | OK | | mbedtls_sha256_self_test | 1 | 0 | OK | | mbedtls_sha512_self_test | 1 | 0 | OK | +---------------------------+--------+--------+--------+

removed from targets.json, added in mbedtls_device.h remove function alternate defines (not used as we replace the full module)

cleanup in include files (unrequired removed + other moved to aes_alt.h) hcryp_aes moved to mbedtls_aes_context to allow multi instances remove ctx->nr, ctx->buf doxygen comments are removed (kept in .h file) function _ALT are removed (full module _ALT) handle error returned by HAL_CRYPxx functions aes is symetric, remove the dupplicated set_key_enc and set_key_dec buffer, and factorize the call to set_key function

adustm · 2017-02-28T17:11:42Z

Hello, I've removed the mbedtls_printf error notifications
I did not change the thread safety nor the code dupplicity as they will be treated separately.
Kind regards

simonbutcher · 2017-02-28T22:50:20Z

@adustm - Thanks. The PR is fine by me now. There are outstanding changes but we should revisit these when we've done the necessary changes to mbed TLS.

adustm · 2017-03-03T09:29:21Z

Hello @0xc0170 I think the 'need review' can be transformed into 'ready to merge'... :)

RonEld · 2017-03-06T09:43:07Z

Hi @adustm
I believe your question on the thread safety should be referred to @andresag01

adustm · 2017-03-07T09:06:14Z

Hello @andresag01, @sbutcher-arm @RonEld
About the comment from Andres

There have been some improvements, but I do not think that this code is thread-safe. Normally, we achieve this in mbed TLS by using the threading API (threading.h). Please note that there is a PR to add support for this: #3150

I have read PR#3150 but it is not clear to me what you are waiting for: shall I wait until this thread PR is integrated in mbed-tls then in mbed-os ? What would be the impact to my code ?

Or can it be treated separately in another PR later on ? I have work about AES for 2 other platforms + SHA1 and SHA256 that are pending the integration of the current PR. I would like to be able to progress on the subject.
I think 0xc0170 is waiting for your green light.
Kind regards

simonbutcher · 2017-03-07T10:10:37Z

As I said above, @andresag01's points are valid, but I don't think we should hold up this PR for the work on thread safety it depends on.

@0xc0170 - this PR is ready to merge. We know it has issues, but they're dependent on changes in mbed TLS and can be fixed later.

0xc0170 · 2017-03-07T10:17:15Z

/morph test-nightly

mbed-bot · 2017-03-07T12:05:00Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1648

Build failed!

screamerbg · 2017-03-09T10:52:54Z

/morph test

mbed-bot · 2017-03-09T12:02:59Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1660

Build failed!

0xc0170 · 2017-03-09T12:35:05Z

@bridadan Need you eye. The failures are about sections overflow.

@adustm The selftest change should not affect other targets or? You can view the failures : http://mbedosci.cloudapp.net/results/pr/3691/1660

From the failures, looks like the test does not fit to many devices that are tested.

adustm · 2017-03-09T12:47:37Z

Hello,
I can see that some failure are due to code size. I've added AES test in the self test to show how I have validated the modification, but I can remove this file from the PR.

adustm · 2017-03-09T13:11:19Z

Thanks @0xc0170 for sharing the build output failure.
I have fixed it. It should be ok now.
Cheers

0xc0170 · 2017-03-09T15:23:25Z

/morph test-nightly

mbed-bot · 2017-03-09T18:06:56Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1665

All builds and test passed!

Ports for Upcoming Targets 3934: [Silicon Labs] Update to HAL and devices ARMmbed/mbed-os#3934 Known Issues There is an issue with LPC1768 failing the 'Semihost file system' test with this release. Fixes and Changes 3691: [TLS / hw acceleration] AES ECB for NUCLEO_F439ZI ARMmbed/mbed-os#3691 3869: NCS36510: Default range changed from 0 to 950mV - ADC ARMmbed/mbed-os#3869 3893: [STM32F7] Update STM32 Cube version v1.6.0 ARMmbed/mbed-os#3893 3917: Fix mistake register setting in serial_format() ARMmbed/mbed-os#3917 3927: [DELTA_DFBM_NQ620] Add RC calibration setting and revise mbed_overrides.c ARMmbed/mbed-os#3927 3918: [NUC472/M453] Support unique locally administered MAC address and other driver updates ARMmbed/mbed-os#3918 3920: Heap size adjusted to work for both tls-client and mbed-client ARMmbed/mbed-os#3920 3969: NUCLEO_F302R8: Add missing PB_8/PB_9 CAN pins ARMmbed/mbed-os#3969

0xc0170 requested a review from simonbutcher February 6, 2017 10:04

0xc0170 added the needs: review label Feb 6, 2017

andresag01 suggested changes Feb 15, 2017

View reviewed changes

andresag01 reviewed Feb 23, 2017

View reviewed changes

simonbutcher suggested changes Feb 28, 2017

View reviewed changes

adustm added 4 commits February 28, 2017 16:41

Add AES_CBC mode

fa8f6e4

Better use of MBEDTLS_AES_ALT

fe1e7aa

removed from targets.json, added in mbedtls_device.h remove function alternate defines (not used as we replace the full module)

adustm force-pushed the STM_aes_f439zi branch from f57b166 to ea71724 Compare February 28, 2017 17:06

remove mbedtls_printf error notification

5c858a4

adustm force-pushed the STM_aes_f439zi branch from ea71724 to 5c858a4 Compare February 28, 2017 17:07

0xc0170 added needs: CI and removed needs: review labels Mar 7, 2017

Revert mbedtls selftest to avoid impact on other targets

9196179

0xc0170 added ready for merge and removed needs: CI labels Mar 14, 2017

adbridge added release-version: 5.4.2 and removed ready for merge labels Mar 14, 2017

adbridge merged commit e743a26 into ARMmbed:master Mar 14, 2017

adustm deleted the STM_aes_f439zi branch May 29, 2017 08:40

[TLS / hw acceleration] AES ECB for NUCLEO_F439ZI #3691

[TLS / hw acceleration] AES ECB for NUCLEO_F439ZI #3691

Uh oh!

Conversation

adustm commented Feb 3, 2017

Description

Status

Steps to test or reproduce

Uh oh!

jeromecoutant commented Feb 3, 2017

Uh oh!

adustm commented Feb 6, 2017

Uh oh!

0xc0170 commented Feb 6, 2017

Uh oh!

screamerbg commented Feb 9, 2017

Uh oh!

adustm commented Feb 14, 2017

Uh oh!

simonbutcher commented Feb 14, 2017

Uh oh!

adustm commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonbutcher commented Feb 14, 2017

Uh oh!

andresag01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

screamerbg commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

adustm commented Feb 14, 2017 •

edited

Loading

screamerbg commented Feb 16, 2017 •

edited

Loading

0xc0170 commented Feb 23, 2017 •

edited

Loading

andresag01 left a comment •

edited

Loading