From 936279e1ffe6bf7e842c46f9a94d98a48dce6754 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Wed, 29 Mar 2017 09:31:18 -0700 Subject: [PATCH] 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 (cherry picked from commit 96d3573cda6f76bcbfc277e69d94914a565218d8) --- audio/2.0/default/Device.cpp | 12 ++++++++++-- audio/2.0/default/Device.h | 2 ++ audio/2.0/default/StreamIn.cpp | 5 ++--- audio/2.0/default/StreamIn.h | 9 +++++---- audio/2.0/default/StreamOut.cpp | 5 ++--- audio/2.0/default/StreamOut.h | 9 +++++---- 6 files changed, 26 insertions(+), 16 deletions(-) diff --git a/audio/2.0/default/Device.cpp b/audio/2.0/default/Device.cpp index 8a51cd7d..5ced0bcb 100644 --- a/audio/2.0/default/Device.cpp +++ b/audio/2.0/default/Device.cpp @@ -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 Device::openOutputStream( ALOGV("open_output_stream status %d stream %p", status, halStream); sp 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 Device::openInputStream( ALOGV("open_input_stream status %d stream %p", status, halStream); sp streamIn; if (status == OK) { - streamIn = new StreamIn(mDevice, halStream); + streamIn = new StreamIn(this, halStream); } AudioConfig suggestedConfig; HidlUtils::audioConfigFromHal(halConfig, &suggestedConfig); diff --git a/audio/2.0/default/Device.h b/audio/2.0/default/Device.h index 46177fc9..77383610 100644 --- a/audio/2.0/default/Device.h +++ b/audio/2.0/default/Device.h @@ -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: diff --git a/audio/2.0/default/StreamIn.cpp b/audio/2.0/default/StreamIn.cpp index b641e823..2745607e 100644 --- a/audio/2.0/default/StreamIn.cpp +++ b/audio/2.0/default/StreamIn.cpp @@ -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, audio_stream_in_t* stream) : mIsClosed(false), mDevice(device), mStream(stream), mStreamCommon(new Stream(&stream->common)), mStreamMmap(new StreamMmap(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. diff --git a/audio/2.0/default/StreamIn.h b/audio/2.0/default/StreamIn.h index b8673874..950d68fc 100644 --- a/audio/2.0/default/StreamIn.h +++ b/audio/2.0/default/StreamIn.h @@ -27,6 +27,7 @@ #include #include +#include "Device.h" #include "Stream.h" namespace android { @@ -55,7 +56,7 @@ struct StreamIn : public IStreamIn { typedef MessageQueue DataMQ; typedef MessageQueue StatusMQ; - StreamIn(audio_hw_device_t* device, audio_stream_in_t* stream); + StreamIn(const sp& device, audio_stream_in_t* stream); // Methods from ::android::hardware::audio::V2_0::IStream follow. Return getFrameSize() override; @@ -101,10 +102,10 @@ struct StreamIn : public IStreamIn { private: bool mIsClosed; - audio_hw_device_t *mDevice; + const sp mDevice; audio_stream_in_t *mStream; - sp mStreamCommon; - sp> mStreamMmap; + const sp mStreamCommon; + const sp> mStreamMmap; std::unique_ptr mCommandMQ; std::unique_ptr mDataMQ; std::unique_ptr mStatusMQ; diff --git a/audio/2.0/default/StreamOut.cpp b/audio/2.0/default/StreamOut.cpp index d820f3ce..88045a0a 100644 --- a/audio/2.0/default/StreamOut.cpp +++ b/audio/2.0/default/StreamOut.cpp @@ -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, audio_stream_out_t* stream) : mIsClosed(false), mDevice(device), mStream(stream), mStreamCommon(new Stream(&stream->common)), mStreamMmap(new StreamMmap(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. diff --git a/audio/2.0/default/StreamOut.h b/audio/2.0/default/StreamOut.h index bbe64a17..99352bc3 100644 --- a/audio/2.0/default/StreamOut.h +++ b/audio/2.0/default/StreamOut.h @@ -27,6 +27,7 @@ #include #include +#include "Device.h" #include "Stream.h" namespace android { @@ -57,7 +58,7 @@ struct StreamOut : public IStreamOut { typedef MessageQueue DataMQ; typedef MessageQueue StatusMQ; - StreamOut(audio_hw_device_t* device, audio_stream_out_t* stream); + StreamOut(const sp& device, audio_stream_out_t* stream); // Methods from ::android::hardware::audio::V2_0::IStream follow. Return getFrameSize() override; @@ -112,10 +113,10 @@ struct StreamOut : public IStreamOut { private: bool mIsClosed; - audio_hw_device_t *mDevice; + const sp mDevice; audio_stream_out_t *mStream; - sp mStreamCommon; - sp> mStreamMmap; + const sp mStreamCommon; + const sp> mStreamMmap; sp mCallback; std::unique_ptr mCommandMQ; std::unique_ptr mDataMQ; -- 2.11.0