search

sparkfun / Arduino_Apollo3

Arduino core to support the Apollo3 microcontroller from Ambiq Micro
83 stars 39 forks source link

use-after-free error in Wire library #454

Open nigelb opened 2 years ago

nigelb commented 2 years ago

Subject of the issue

use-after-free error in Wire library.

The relevant methods are Wire.begin: https://github.com/sparkfun/Arduino_Apollo3/blob/4e24a0275c4b84b0f3162e811162899dae4a1c62/libraries/Wire/src/Wire.cpp#L14-L19

and Wire.end: https://github.com/sparkfun/Arduino_Apollo3/blob/4e24a0275c4b84b0f3162e811162899dae4a1c62/libraries/Wire/src/Wire.cpp#L31-L40

With the sequence:

    Wire.begin();
    Wire.end();
    Wire.begin();
   //I2C Activity

After the call to Wire.end, master is pointing to the location of the deleted mbed::I2C object.

Your workbench

Steps to reproduce

Example Code

#include "Wire.h"

arduino::MbedI2C *myWire;

void setup() {
  Serial.begin(115200);
  Serial.println("Wire Use-After-Free");
  Wire.end();
  myWire = new arduino::MbedI2C(40, 39);
}

void loop() {
  Serial.println("Loop");
  myWire->begin();
  Serial.println("Begin succeeded!");

  Serial.println("Starting I2C transmission");
  myWire->beginTransmission(0x48); //TMP117
  myWire->endTransmission();
  Serial.println("I2C transmission succeeded!");

  myWire->end();
}

Example Code Output

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission

++ MbedOS Fault Handler ++

FaultType: HardFault

Context:
R0: 1000A750
R1: 90
R2: 1000A584
R3: A3009150
R4: 1000A750
R5: 0
R6: 0
R7: 0
R8: 0
R9: 0
R10: 0
R11: 0
R12: 23949
SP   : 10006E48
LR   : 10F2D
PC   : A3009150
xPSR : 0
PSP  : 10006DE0
MSP  : 1005FF70
CPUID: 410FC241
HFSR : 40000000
MMFSR: 1
BFSR : 0
UFSR : 0
DFSR : 0
AFSR : 0
Mode : Thread
Priv : Privileged
Stack: PSP

-- MbedOS Fault Handler --

++ MbedOS Error Info ++
Error Status: 0x80FF013D Code: 317 Module: 255
Error Message: Fault exception
Location: 0xA3009150
Error Value: 0x10005E30
Current Thread: main Id: 0x10004248 Entry: 0x22C75 StackSize: 0x1000 StackMem: 0x10005E90 SP: 0x10006E48 
For more info, visit: https://mbed.com/s/error?error=0x80FF013D&tgt=SFE_ARTEMIS_ATP
-- MbedOS Error Info --

Trace of I2C Output

First I2C transmission definitely worked: image

Possible solution

This code would stop the use-after-free. Maybe more is needed.

 void arduino::MbedI2C::end() { 
    if (master != NULL) { 
        delete master; 
        master = NULL;
    } 
 #ifdef DEVICE_I2CSLAVE 
    if (slave != NULL) { 
        delete slave; 
        slave = NULL; 
    } 
 #endif 
 }
Wenn0101 commented 2 years ago

Great find! This wont just affect Wire, I know SPI has similar logic, and I will have to comb thru the codebase for other similar mistakes.

The suggested fix is a great start that fixes the use-after-free, but leads to another runtime error. As pointed out in https://github.com/sparkfun/Arduino_Apollo3/issues/439 mbed::I2C does not have a destructor that calls the de-init for wire, none of the mbed objects do. So when the new wire is created, the HAL complains that the module has already been inited (and not deinited) and fails.

I am tempted to just add destructors for all the mbed classes that Artemis uses in the Sparkfun branch of mbed, but I fear that this will be to far a departure from the main branch to ever update again later. I may end up putting in a solution as @paulvha has suggested for the OpenLog in https://github.com/sparkfun/OpenLog_Artemis/issues/117. This "breaks" some of the encapsulation of the mbed core, but isn't nearly as large of a change.

I will think on this a bit, but I may still try to sneak a fix in to the upcoming v2.2.1.

nigelb commented 2 years ago

Hi @Wenn0101 thanks for looking at this. If you created the destructors could you push them upstream to mbed? It might just be me, but it seems odd that they don't have them.

Also while we are on the topic of hardware de-initialization neither UART::~UART, nor UART::end properly de-initialize the UART hardware. #412, and maybe #411, seem to be in this wheelhouse as well.

Wenn0101 commented 2 years ago

Looking at the Mbed issue, they seem to be reluctant to bring in these changes. Because the feature has been missing for so long, many of the targets never implemented the de-init functions for the various drivers. So if they added it in, it would lead to a situation where some of the target support it and other do not. It could be worth worth a try to see if they would though.

I think that this will require more thought then a last minute change, so I am not going to try and include this in v2.2.1.

nigelb commented 2 years ago

Hi @Wenn0101, I think I have come up with a shim that allows us to have our cake and eat it too. With this you won't have to push code to mbed or maintain a forked version of their code.

Firstly make the changes I suggested above:

 void arduino::MbedI2C::end() { 
    if (master != NULL) { 
        delete master; 
        master = NULL;
    } 
 #ifdef DEVICE_I2CSLAVE 
    if (slave != NULL) { 
        delete slave; 
        slave = NULL; 
    } 
 #endif 
 }

At this point, if we compile and run my example from above we get this:

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop

++ MbedOS Error Info ++
Error Status: 0x80FF0144 Code: 324 Module: 255
Error Message: Assertion failed: AM_HAL_STATUS_SUCCESS == am_hal_iom_initialize(obj->iom.inst, &obj->iom.handle)
Location: 0x21C9D
File: mbed-os/targets/TARGET_Ambiq_Micro/TARGET_Apollo3/device/iom_api.c+33
Error Value: 0x0
Current Thread: main Id: 0x10004248 Entry: 0x22C85 StackSize: 0x1000 StackMem: 0x10005E90 SP: 0x10006DE4 
For more info, visit: https://mbed.com/s/error?error=0x80FF0144&tgt=SFE_ARTEMIS_ATP
-- MbedOS Error Info --

However if we create a new class derived from mbed::I2C by changing: https://github.com/sparkfun/Arduino_Apollo3/blob/4e24a0275c4b84b0f3162e811162899dae4a1c62/libraries/Wire/src/Wire.h#L20-L22

to:

namespace arduino {

class myI2C: public mbed::I2C
{
    public:
    using mbed::I2C::I2C;
    virtual ~myI2C(){
        i2c_free(&_i2c);
    };
};

class MbedI2C : public HardwareI2C

\ \ then replace this: https://github.com/sparkfun/Arduino_Apollo3/blob/4e24a0275c4b84b0f3162e811162899dae4a1c62/libraries/Wire/src/Wire.h#L55

with:

    myI2C*      master;

\ \ and this line: https://github.com/sparkfun/Arduino_Apollo3/blob/4e24a0275c4b84b0f3162e811162899dae4a1c62/libraries/Wire/src/Wire.cpp#L16

with this:

    master = new myI2C((PinName)_sda, (PinName)_scl);

\ \ Now if we compile and run this we get the following output:

Wire Use-After-Free
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
Loop
Begin succeeded!
Starting I2C transmission
I2C transmission succeeded!
.
.
.

The myI2C class probably needs a more appropriate name, and I am not 100% certain that the hardware fully de-initialized, however this does demonstrate that we can get hold of the i2c_t object so we can call i2c_free with it.