Migrate IDisplayEventConnection interface to AIDL
This addresses security vulnerabilities due to hard coded binder
interface.
Bug: 195660647
Test: atest services/surfaceflinger/tests/unittests/SchedulerTest.cpp
Change-Id: I948e97e37056286d54623ca6232580187b138e62
diff --git a/libs/binder/include/binder/IInterface.h b/libs/binder/include/binder/IInterface.h
index 415b19a..f5abb85 100644
--- a/libs/binder/include/binder/IInterface.h
+++ b/libs/binder/include/binder/IInterface.h
@@ -228,7 +228,6 @@
"android.gfx.tests.IIPCTest",
"android.gfx.tests.ISafeInterfaceTest",
"android.graphicsenv.IGpuService",
- "android.gui.DisplayEventConnection",
"android.gui.IConsumerListener",
"android.gui.IGraphicBufferConsumer",
"android.gui.ITransactionComposerListener",
diff --git a/libs/gui/Android.bp b/libs/gui/Android.bp
index fea9f81..c5dec19 100644
--- a/libs/gui/Android.bp
+++ b/libs/gui/Android.bp
@@ -180,7 +180,6 @@
"FrameTimelineInfo.cpp",
"GLConsumer.cpp",
"IConsumerListener.cpp",
- "IDisplayEventConnection.cpp",
"IGraphicBufferConsumer.cpp",
"IGraphicBufferProducer.cpp",
"IProducerListener.cpp",
diff --git a/libs/gui/DisplayEventReceiver.cpp b/libs/gui/DisplayEventReceiver.cpp
index 03b33c7..b916e48 100644
--- a/libs/gui/DisplayEventReceiver.cpp
+++ b/libs/gui/DisplayEventReceiver.cpp
@@ -19,7 +19,6 @@
#include <utils/Errors.h>
#include <gui/DisplayEventReceiver.h>
-#include <gui/IDisplayEventConnection.h>
#include <gui/ISurfaceComposer.h>
#include <private/gui/ComposerService.h>
diff --git a/libs/gui/IDisplayEventConnection.cpp b/libs/gui/IDisplayEventConnection.cpp
deleted file mode 100644
index c0e246f..0000000
--- a/libs/gui/IDisplayEventConnection.cpp
+++ /dev/null
@@ -1,80 +0,0 @@
-/*
- * Copyright (C) 2011 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <gui/IDisplayEventConnection.h>
-
-#include <private/gui/BitTube.h>
-
-namespace android {
-
-namespace { // Anonymous
-
-enum class Tag : uint32_t {
- STEAL_RECEIVE_CHANNEL = IBinder::FIRST_CALL_TRANSACTION,
- SET_VSYNC_RATE,
- REQUEST_NEXT_VSYNC,
- LAST = REQUEST_NEXT_VSYNC,
-};
-
-} // Anonymous namespace
-
-class BpDisplayEventConnection : public SafeBpInterface<IDisplayEventConnection> {
-public:
- explicit BpDisplayEventConnection(const sp<IBinder>& impl)
- : SafeBpInterface<IDisplayEventConnection>(impl, "BpDisplayEventConnection") {}
-
- ~BpDisplayEventConnection() override;
-
- status_t stealReceiveChannel(gui::BitTube* outChannel) override {
- return callRemote<decltype(
- &IDisplayEventConnection::stealReceiveChannel)>(Tag::STEAL_RECEIVE_CHANNEL,
- outChannel);
- }
-
- status_t setVsyncRate(uint32_t count) override {
- return callRemote<decltype(&IDisplayEventConnection::setVsyncRate)>(Tag::SET_VSYNC_RATE,
- count);
- }
-
- void requestNextVsync() override {
- callRemoteAsync<decltype(&IDisplayEventConnection::requestNextVsync)>(
- Tag::REQUEST_NEXT_VSYNC);
- }
-};
-
-// Out-of-line virtual method definition to trigger vtable emission in this translation unit (see
-// clang warning -Wweak-vtables)
-BpDisplayEventConnection::~BpDisplayEventConnection() = default;
-
-IMPLEMENT_META_INTERFACE(DisplayEventConnection, "android.gui.DisplayEventConnection");
-
-status_t BnDisplayEventConnection::onTransact(uint32_t code, const Parcel& data, Parcel* reply,
- uint32_t flags) {
- if (code < IBinder::FIRST_CALL_TRANSACTION || code > static_cast<uint32_t>(Tag::LAST)) {
- return BBinder::onTransact(code, data, reply, flags);
- }
- auto tag = static_cast<Tag>(code);
- switch (tag) {
- case Tag::STEAL_RECEIVE_CHANNEL:
- return callLocal(data, reply, &IDisplayEventConnection::stealReceiveChannel);
- case Tag::SET_VSYNC_RATE:
- return callLocal(data, reply, &IDisplayEventConnection::setVsyncRate);
- case Tag::REQUEST_NEXT_VSYNC:
- return callLocalAsync(data, reply, &IDisplayEventConnection::requestNextVsync);
- }
-}
-
-} // namespace android
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index a90f755..7f73013 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -17,12 +17,12 @@
// tag as surfaceflinger
#define LOG_TAG "SurfaceFlinger"
+#include <android/gui/IDisplayEventConnection.h>
#include <android/gui/IRegionSamplingListener.h>
#include <android/gui/ITransactionTraceListener.h>
#include <binder/IPCThreadState.h>
#include <binder/IServiceManager.h>
#include <binder/Parcel.h>
-#include <gui/IDisplayEventConnection.h>
#include <gui/IGraphicBufferProducer.h>
#include <gui/ISurfaceComposer.h>
#include <gui/ISurfaceComposerClient.h>
@@ -44,6 +44,7 @@
namespace android {
+using gui::IDisplayEventConnection;
using gui::IRegionSamplingListener;
using gui::IWindowInfosListener;
using ui::ColorMode;
diff --git a/libs/gui/aidl/android/gui/BitTube.aidl b/libs/gui/aidl/android/gui/BitTube.aidl
new file mode 100644
index 0000000..6b0595e
--- /dev/null
+++ b/libs/gui/aidl/android/gui/BitTube.aidl
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.gui;
+
+parcelable BitTube cpp_header "private/gui/BitTube.h";
diff --git a/libs/gui/aidl/android/gui/IDisplayEventConnection.aidl b/libs/gui/aidl/android/gui/IDisplayEventConnection.aidl
new file mode 100644
index 0000000..9f41593
--- /dev/null
+++ b/libs/gui/aidl/android/gui/IDisplayEventConnection.aidl
@@ -0,0 +1,41 @@
+/*
+ * Copyright 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.gui;
+
+import android.gui.BitTube;
+
+/** @hide */
+interface IDisplayEventConnection {
+ /*
+ * stealReceiveChannel() returns a BitTube to receive events from. Only the receive file
+ * descriptor of outChannel will be initialized, and this effectively "steals" the receive
+ * channel from the remote end (such that the remote end can only use its send channel).
+ */
+ void stealReceiveChannel(out BitTube outChannel);
+
+ /*
+ * setVsyncRate() sets the vsync event delivery rate. A value of 1 returns every vsync event.
+ * A value of 2 returns every other event, etc. A value of 0 returns no event unless
+ * requestNextVsync() has been called.
+ */
+ void setVsyncRate(in int count);
+
+ /*
+ * requestNextVsync() schedules the next vsync event. It has no effect if the vsync rate is > 0.
+ */
+ oneway void requestNextVsync(); // Asynchronous
+}
diff --git a/libs/gui/include/gui/DisplayEventReceiver.h b/libs/gui/include/gui/DisplayEventReceiver.h
index ca36843..456bbfb 100644
--- a/libs/gui/include/gui/DisplayEventReceiver.h
+++ b/libs/gui/include/gui/DisplayEventReceiver.h
@@ -33,7 +33,7 @@
// ----------------------------------------------------------------------------
-class IDisplayEventConnection;
+using gui::IDisplayEventConnection;
namespace gui {
class BitTube;
diff --git a/libs/gui/include/gui/IDisplayEventConnection.h b/libs/gui/include/gui/IDisplayEventConnection.h
deleted file mode 100644
index cff22a3..0000000
--- a/libs/gui/include/gui/IDisplayEventConnection.h
+++ /dev/null
@@ -1,65 +0,0 @@
-/*
- * Copyright (C) 2011 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#pragma once
-
-#include <binder/IInterface.h>
-#include <binder/SafeInterface.h>
-#include <gui/ISurfaceComposer.h>
-#include <utils/Errors.h>
-
-#include <cstdint>
-
-namespace android {
-
-namespace gui {
-class BitTube;
-} // namespace gui
-
-class IDisplayEventConnection : public IInterface {
-public:
- DECLARE_META_INTERFACE(DisplayEventConnection)
-
- /*
- * stealReceiveChannel() returns a BitTube to receive events from. Only the receive file
- * descriptor of outChannel will be initialized, and this effectively "steals" the receive
- * channel from the remote end (such that the remote end can only use its send channel).
- */
- virtual status_t stealReceiveChannel(gui::BitTube* outChannel) = 0;
-
- /*
- * setVsyncRate() sets the vsync event delivery rate. A value of 1 returns every vsync event.
- * A value of 2 returns every other event, etc. A value of 0 returns no event unless
- * requestNextVsync() has been called.
- */
- virtual status_t setVsyncRate(uint32_t count) = 0;
-
- /*
- * requestNextVsync() schedules the next vsync event. It has no effect if the vsync rate is > 0.
- */
- virtual void requestNextVsync() = 0; // Asynchronous
-};
-
-class BnDisplayEventConnection : public SafeBnInterface<IDisplayEventConnection> {
-public:
- BnDisplayEventConnection()
- : SafeBnInterface<IDisplayEventConnection>("BnDisplayEventConnection") {}
-
- status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply,
- uint32_t flags = 0) override;
-};
-
-} // namespace android
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index e1e9182..2546e4c 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -17,6 +17,7 @@
#pragma once
#include <android/gui/DisplayBrightness.h>
+#include <android/gui/IDisplayEventConnection.h>
#include <android/gui/IFpsListener.h>
#include <android/gui/IHdrLayerInfoListener.h>
#include <android/gui/IRegionSamplingListener.h>
@@ -61,12 +62,12 @@
struct LayerCaptureArgs;
class LayerDebugInfo;
class HdrCapabilities;
-class IDisplayEventConnection;
class IGraphicBufferProducer;
class ISurfaceComposerClient;
class Rect;
enum class FrameEvent;
+using gui::IDisplayEventConnection;
using gui::IRegionSamplingListener;
using gui::IScreenCaptureListener;
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index e0c2aa7..d6ac3f9 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -19,11 +19,11 @@
#include <gtest/gtest.h>
#include <SurfaceFlingerProperties.h>
+#include <android/gui/IDisplayEventConnection.h>
#include <android/hardware/configstore/1.0/ISurfaceFlingerConfigs.h>
#include <binder/ProcessState.h>
#include <configstore/Utils.h>
#include <gui/BufferItemConsumer.h>
-#include <gui/IDisplayEventConnection.h>
#include <gui/IProducerListener.h>
#include <gui/ISurfaceComposer.h>
#include <gui/Surface.h>
@@ -45,6 +45,7 @@
// retrieve wide-color and hdr settings from configstore
using namespace android::hardware::configstore;
using namespace android::hardware::configstore::V1_0;
+using gui::IDisplayEventConnection;
using gui::IRegionSamplingListener;
using ui::ColorMode;
diff --git a/services/surfaceflinger/Scheduler/EventThread.cpp b/services/surfaceflinger/Scheduler/EventThread.cpp
index 455289f..627c49a 100644
--- a/services/surfaceflinger/Scheduler/EventThread.cpp
+++ b/services/surfaceflinger/Scheduler/EventThread.cpp
@@ -170,20 +170,21 @@
mEventThread->registerDisplayEventConnection(this);
}
-status_t EventThreadConnection::stealReceiveChannel(gui::BitTube* outChannel) {
+binder::Status EventThreadConnection::stealReceiveChannel(gui::BitTube* outChannel) {
outChannel->setReceiveFd(mChannel.moveReceiveFd());
outChannel->setSendFd(base::unique_fd(dup(mChannel.getSendFd())));
- return NO_ERROR;
+ return binder::Status::ok();
}
-status_t EventThreadConnection::setVsyncRate(uint32_t rate) {
- mEventThread->setVsyncRate(rate, this);
- return NO_ERROR;
+binder::Status EventThreadConnection::setVsyncRate(int rate) {
+ mEventThread->setVsyncRate(static_cast<uint32_t>(rate), this);
+ return binder::Status::ok();
}
-void EventThreadConnection::requestNextVsync() {
+binder::Status EventThreadConnection::requestNextVsync() {
ATRACE_NAME("requestNextVsync");
mEventThread->requestNextVsync(this);
+ return binder::Status::ok();
}
status_t EventThreadConnection::postEvent(const DisplayEventReceiver::Event& event) {
diff --git a/services/surfaceflinger/Scheduler/EventThread.h b/services/surfaceflinger/Scheduler/EventThread.h
index de43570..fa9af09 100644
--- a/services/surfaceflinger/Scheduler/EventThread.h
+++ b/services/surfaceflinger/Scheduler/EventThread.h
@@ -17,8 +17,8 @@
#pragma once
#include <android-base/thread_annotations.h>
+#include <android/gui/BnDisplayEventConnection.h>
#include <gui/DisplayEventReceiver.h>
-#include <gui/IDisplayEventConnection.h>
#include <private/gui/BitTube.h>
#include <sys/types.h>
#include <utils/Errors.h>
@@ -80,7 +80,7 @@
virtual void dump(std::string& result) const = 0;
};
-class EventThreadConnection : public BnDisplayEventConnection {
+class EventThreadConnection : public gui::BnDisplayEventConnection {
public:
EventThreadConnection(EventThread*, uid_t callingUid, ResyncCallback,
ISurfaceComposer::EventRegistrationFlags eventRegistration = {});
@@ -88,9 +88,9 @@
virtual status_t postEvent(const DisplayEventReceiver::Event& event);
- status_t stealReceiveChannel(gui::BitTube* outChannel) override;
- status_t setVsyncRate(uint32_t rate) override;
- void requestNextVsync() override; // asynchronous
+ binder::Status stealReceiveChannel(gui::BitTube* outChannel) override;
+ binder::Status setVsyncRate(int rate) override;
+ binder::Status requestNextVsync() override; // asynchronous
// Called in response to requestNextVsync.
const ResyncCallback resyncCallback;
diff --git a/services/surfaceflinger/Scheduler/MessageQueue.h b/services/surfaceflinger/Scheduler/MessageQueue.h
index dd69d60..9532e26 100644
--- a/services/surfaceflinger/Scheduler/MessageQueue.h
+++ b/services/surfaceflinger/Scheduler/MessageQueue.h
@@ -22,7 +22,7 @@
#include <utility>
#include <android-base/thread_annotations.h>
-#include <gui/IDisplayEventConnection.h>
+#include <android/gui/IDisplayEventConnection.h>
#include <private/gui/BitTube.h>
#include <utils/Looper.h>
#include <utils/Timers.h>
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 14761ea..08d8527 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -26,6 +26,7 @@
#include <android-base/properties.h>
#include <android/configuration.h>
+#include <android/gui/IDisplayEventConnection.h>
#include <android/hardware/configstore/1.0/ISurfaceFlingerConfigs.h>
#include <android/hardware/configstore/1.1/ISurfaceFlingerConfigs.h>
#include <android/hardware/configstore/1.1/types.h>
@@ -51,7 +52,6 @@
#include <ftl/future.h>
#include <gui/BufferQueue.h>
#include <gui/DebugEGLImageTracker.h>
-#include <gui/IDisplayEventConnection.h>
#include <gui/IProducerListener.h>
#include <gui/LayerDebugInfo.h>
#include <gui/LayerMetadata.h>
@@ -169,6 +169,7 @@
using android::hardware::power::Boost;
using base::StringAppendF;
using gui::DisplayInfo;
+using gui::IDisplayEventConnection;
using gui::IWindowInfosListener;
using gui::WindowInfo;
using ui::ColorMode;
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index a6fd378..f48abb7 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -48,9 +48,9 @@
: EventThreadConnection(eventThread, /*callingUid=*/0, ResyncCallback()) {}
~MockEventThreadConnection() = default;
- MOCK_METHOD1(stealReceiveChannel, status_t(gui::BitTube* outChannel));
- MOCK_METHOD1(setVsyncRate, status_t(uint32_t count));
- MOCK_METHOD0(requestNextVsync, void());
+ MOCK_METHOD1(stealReceiveChannel, binder::Status(gui::BitTube* outChannel));
+ MOCK_METHOD1(setVsyncRate, binder::Status(int count));
+ MOCK_METHOD0(requestNextVsync, binder::Status());
};
SchedulerTest();