Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 96d3573c authored by Mikhail Naganov's avatar Mikhail Naganov
Browse files

audiohal: Fix UAF of HAL devices in Stream objects 

Stream objects used to hold a pointer to underlying HAL device
object which they didn't own. Since destruction of server side
objects is asynchronous, it was possible that a Device object
gets destroyed before Stream objects, making all the HAL device
object pointer to become stale.

Fixed by adding a strong reference to Device objects into Stream
objects.

Bug: 36702804
Change-Id: I3da3611afbb91d6fd6410ac5b8af2a2eebfa6dac
Test: ran Loopback app and HAL VTS tests
parent b01e1a8d

audio/2.0/default/Device.cpp

+10 −2
Original line number Diff line number Diff line
@@ -59,6 +59,14 @@ Result Device::analyzeStatus(const char* funcName, int status) { 
    }

}



void Device::closeInputStream(audio_stream_in_t* stream) {

    mDevice->close_input_stream(mDevice, stream);

}



void Device::closeOutputStream(audio_stream_out_t* stream) {

    mDevice->close_output_stream(mDevice, stream);

}



char* Device::halGetParameters(const char* keys) {

    return mDevice->get_parameters(mDevice, keys);

}
@@ -160,7 +168,7 @@ Return<void> Device::openOutputStream( 
    ALOGV("open_output_stream status %d stream %p", status, halStream);

    sp<IStreamOut> streamOut;

    if (status == OK) {

        streamOut = new StreamOut(mDevice, halStream);

        streamOut = new StreamOut(this, halStream);

    }

    AudioConfig suggestedConfig;

    HidlUtils::audioConfigFromHal(halConfig, &suggestedConfig);
@@ -196,7 +204,7 @@ Return<void> Device::openInputStream( 
    ALOGV("open_input_stream status %d stream %p", status, halStream);

    sp<IStreamIn> streamIn;

    if (status == OK) {

        streamIn = new StreamIn(mDevice, halStream);

        streamIn = new StreamIn(this, halStream);

    }

    AudioConfig suggestedConfig;

    HidlUtils::audioConfigFromHal(halConfig, &suggestedConfig);

audio/2.0/default/Device.h

+2 −0
Original line number Diff line number Diff line
@@ -98,6 +98,8 @@ struct Device : public IDevice, public ParametersUtil { 


    // Utility methods for extending interfaces.

    Result analyzeStatus(const char* funcName, int status);

    void closeInputStream(audio_stream_in_t* stream);

    void closeOutputStream(audio_stream_out_t* stream);

    audio_hw_device_t* device() const { return mDevice; }



  private:

audio/2.0/default/StreamIn.cpp

+2 −3
Original line number Diff line number Diff line
@@ -135,7 +135,7 @@ bool ReadThread::threadLoop() { 


}  // namespace



StreamIn::StreamIn(audio_hw_device_t* device, audio_stream_in_t* stream)

StreamIn::StreamIn(const sp<Device>& device, audio_stream_in_t* stream)

        : mIsClosed(false), mDevice(device), mStream(stream),

          mStreamCommon(new Stream(&stream->common)),

          mStreamMmap(new StreamMmap<audio_stream_in_t>(stream)),
@@ -154,9 +154,8 @@ StreamIn::~StreamIn() { 
        status_t status = EventFlag::deleteEventFlag(&mEfGroup);

        ALOGE_IF(status, "read MQ event flag deletion error: %s", strerror(-status));

    }

    mDevice->close_input_stream(mDevice, mStream);

    mDevice->closeInputStream(mStream);

    mStream = nullptr;

    mDevice = nullptr;

}



// Methods from ::android::hardware::audio::V2_0::IStream follow.

audio/2.0/default/StreamIn.h

+5 −4
Original line number Diff line number Diff line
@@ -27,6 +27,7 @@ 
#include <hidl/Status.h>

#include <utils/Thread.h>



#include "Device.h"

#include "Stream.h"



namespace android {
@@ -55,7 +56,7 @@ struct StreamIn : public IStreamIn { 
    typedef MessageQueue<uint8_t, kSynchronizedReadWrite> DataMQ;

    typedef MessageQueue<ReadStatus, kSynchronizedReadWrite> StatusMQ;



    StreamIn(audio_hw_device_t* device, audio_stream_in_t* stream);

    StreamIn(const sp<Device>& device, audio_stream_in_t* stream);



    // Methods from ::android::hardware::audio::V2_0::IStream follow.

    Return<uint64_t> getFrameSize()  override;
@@ -101,10 +102,10 @@ struct StreamIn : public IStreamIn { 


  private:

    bool mIsClosed;

    audio_hw_device_t *mDevice;

    const sp<Device> mDevice;

    audio_stream_in_t *mStream;

    sp<Stream> mStreamCommon;

    sp<StreamMmap<audio_stream_in_t>> mStreamMmap;

    const sp<Stream> mStreamCommon;

    const sp<StreamMmap<audio_stream_in_t>> mStreamMmap;

    std::unique_ptr<CommandMQ> mCommandMQ;

    std::unique_ptr<DataMQ> mDataMQ;

    std::unique_ptr<StatusMQ> mStatusMQ;

audio/2.0/default/StreamOut.cpp

+2 −3
Original line number Diff line number Diff line
@@ -135,7 +135,7 @@ bool WriteThread::threadLoop() { 


}  // namespace



StreamOut::StreamOut(audio_hw_device_t* device, audio_stream_out_t* stream)

StreamOut::StreamOut(const sp<Device>& device, audio_stream_out_t* stream)

        : mIsClosed(false), mDevice(device), mStream(stream),

          mStreamCommon(new Stream(&stream->common)),

          mStreamMmap(new StreamMmap<audio_stream_out_t>(stream)),
@@ -155,9 +155,8 @@ StreamOut::~StreamOut() { 
        ALOGE_IF(status, "write MQ event flag deletion error: %s", strerror(-status));

    }

    mCallback.clear();

    mDevice->close_output_stream(mDevice, mStream);

    mDevice->closeOutputStream(mStream);

    mStream = nullptr;

    mDevice = nullptr;

}



// Methods from ::android::hardware::audio::V2_0::IStream follow.