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

Commit fb4b7377 authored by Dominik Laskowski's avatar Dominik Laskowski
Browse files

SF: Fix UAF on pacesetter change during commit 

During commit, the pacesetter's FrameTargeter could be destroyed after a
hotplug reconnect or a resolution change, via processDisplayChanged. The
reference in Scheduler::onFrameSignal was then dangling, causing a crash
when dereferenced later during composite.

Fixes: 308287117
Test: SchedulerTest.onFrameSignalMultipleDisplays
Change-Id: I413ee7d9967e731825106ef2b6d37fbfb15516ea
parent 81d8aadf

services/surfaceflinger/Scheduler/Scheduler.cpp

+21 −21
Original line number Diff line number Diff line
@@ -181,32 +181,33 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId, 
             .expectedVsyncTime = expectedVsyncTime,

             .sfWorkDuration = mVsyncModulator->getVsyncConfig().sfWorkDuration};



    LOG_ALWAYS_FATAL_IF(!mPacesetterDisplayId);

    const auto pacesetterId = *mPacesetterDisplayId;

    const auto pacesetterOpt = mDisplays.get(pacesetterId);



    FrameTargeter& pacesetterTargeter = *pacesetterOpt->get().targeterPtr;

    pacesetterTargeter.beginFrame(beginFrameArgs, *pacesetterOpt->get().schedulePtr);

    ftl::NonNull<const Display*> pacesetterPtr = pacesetterPtrLocked();

    pacesetterPtr->targeterPtr->beginFrame(beginFrameArgs, *pacesetterPtr->schedulePtr);



    {

        FrameTargets targets;

    targets.try_emplace(pacesetterId, &pacesetterTargeter.target());

        targets.try_emplace(pacesetterPtr->displayId, &pacesetterPtr->targeterPtr->target());



        for (const auto& [id, display] : mDisplays) {

        if (id == pacesetterId) continue;

            if (id == pacesetterPtr->displayId) continue;



            FrameTargeter& targeter = *display.targeterPtr;

            targeter.beginFrame(beginFrameArgs, *display.schedulePtr);

            targets.try_emplace(id, &targeter.target());

        }



    if (!compositor.commit(pacesetterId, targets)) return;

        if (!compositor.commit(pacesetterPtr->displayId, targets)) return;

    }



    // The pacesetter may have changed or been registered anew during commit.

    pacesetterPtr = pacesetterPtrLocked();



    // TODO(b/256196556): Choose the frontrunner display.

    FrameTargeters targeters;

    targeters.try_emplace(pacesetterId, &pacesetterTargeter);

    targeters.try_emplace(pacesetterPtr->displayId, pacesetterPtr->targeterPtr.get());



    for (auto& [id, display] : mDisplays) {

        if (id == pacesetterId) continue;

        if (id == pacesetterPtr->displayId) continue;



        FrameTargeter& targeter = *display.targeterPtr;

        targeters.try_emplace(id, &targeter);
@@ -214,7 +215,7 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId, 


    if (FlagManager::getInstance().vrr_config() &&

        CC_UNLIKELY(mPacesetterFrameDurationFractionToSkip > 0.f)) {

        const auto period = pacesetterTargeter.target().expectedFrameDuration();

        const auto period = pacesetterPtr->targeterPtr->target().expectedFrameDuration();

        const auto skipDuration = Duration::fromNs(

                static_cast<nsecs_t>(period.ns() * mPacesetterFrameDurationFractionToSkip));

        ATRACE_FORMAT("Injecting jank for %f%% of the frame (%" PRId64 " ns)",
@@ -224,19 +225,18 @@ void Scheduler::onFrameSignal(ICompositor& compositor, VsyncId vsyncId, 
    }



    if (FlagManager::getInstance().vrr_config()) {

        const auto minFramePeriod = pacesetterOpt->get().schedulePtr->minFramePeriod();

        const auto minFramePeriod = pacesetterPtr->schedulePtr->minFramePeriod();

        const auto presentFenceForPastVsync =

                pacesetterTargeter.target().presentFenceForPastVsync(minFramePeriod);

                pacesetterPtr->targeterPtr->target().presentFenceForPastVsync(minFramePeriod);

        const auto lastConfirmedPresentTime = presentFenceForPastVsync->getSignalTime();

        if (lastConfirmedPresentTime != Fence::SIGNAL_TIME_PENDING &&

            lastConfirmedPresentTime != Fence::SIGNAL_TIME_INVALID) {

            pacesetterOpt->get()

                    .schedulePtr->getTracker()

            pacesetterPtr->schedulePtr->getTracker()

                    .onFrameBegin(expectedVsyncTime, TimePoint::fromNs(lastConfirmedPresentTime));

        }

    }



    const auto resultsPerDisplay = compositor.composite(pacesetterId, targeters);

    const auto resultsPerDisplay = compositor.composite(pacesetterPtr->displayId, targeters);

    compositor.sample();



    for (const auto& [id, targeter] : targeters) {

services/surfaceflinger/Scheduler/Scheduler.h

+6 −1
Original line number Diff line number Diff line
@@ -33,6 +33,7 @@ 
#pragma clang diagnostic pop // ignored "-Wconversion -Wextra"



#include <ftl/fake_guard.h>

#include <ftl/non_null.h>

#include <ftl/optional.h>

#include <scheduler/Features.h>

#include <scheduler/FrameTargeter.h>
@@ -516,13 +517,17 @@ private: 
                                                     });

    }



    // The pacesetter must exist as a precondition.

    ftl::NonNull<const Display*> pacesetterPtrLocked() const REQUIRES(mDisplayLock) {

        return ftl::as_non_null(&pacesetterDisplayLocked()->get());

    }



    RefreshRateSelectorPtr pacesetterSelectorPtr() const EXCLUDES(mDisplayLock) {

        std::scoped_lock lock(mDisplayLock);

        return pacesetterSelectorPtrLocked();

    }



    RefreshRateSelectorPtr pacesetterSelectorPtrLocked() const REQUIRES(mDisplayLock) {

        ftl::FakeGuard guard(kMainThreadContext);

        return pacesetterDisplayLocked()

                .transform([](const Display& display) { return display.selectorPtr; })

                .or_else([] { return std::optional<RefreshRateSelectorPtr>(nullptr); })

services/surfaceflinger/tests/unittests/SchedulerTest.cpp

+58 −13
Original line number Diff line number Diff line
@@ -457,26 +457,51 @@ TEST_F(SchedulerTest, onFrameSignalMultipleDisplays) { 
    using VsyncIds = std::vector<std::pair<PhysicalDisplayId, VsyncId>>;



    struct Compositor final : ICompositor {

        VsyncIds vsyncIds;

        explicit Compositor(TestableScheduler& scheduler) : scheduler(scheduler) {}



        TestableScheduler& scheduler;



        struct {

            PhysicalDisplayId commit;

            PhysicalDisplayId composite;

        } pacesetterIds;



        struct {

            VsyncIds commit;

            VsyncIds composite;

        } vsyncIds;



        bool committed = true;

        bool changePacesetter = false;



        void configure() override {}



        bool commit(PhysicalDisplayId, const scheduler::FrameTargets& targets) override {

            vsyncIds.clear();

        bool commit(PhysicalDisplayId pacesetterId,

                    const scheduler::FrameTargets& targets) override {

            pacesetterIds.commit = pacesetterId;



            vsyncIds.commit.clear();

            vsyncIds.composite.clear();



            for (const auto& [id, target] : targets) {

                vsyncIds.emplace_back(id, target->vsyncId());

                vsyncIds.commit.emplace_back(id, target->vsyncId());

            }



            if (changePacesetter) {

                scheduler.setPacesetterDisplay(kDisplayId2);

            }



            return committed;

        }



        CompositeResultsPerDisplay composite(PhysicalDisplayId,

                                             const scheduler::FrameTargeters&) override {

        CompositeResultsPerDisplay composite(PhysicalDisplayId pacesetterId,

                                             const scheduler::FrameTargeters& targeters) override {

            pacesetterIds.composite = pacesetterId;



            CompositeResultsPerDisplay results;



            for (const auto& [id, _] : vsyncIds) {

            for (const auto& [id, targeter] : targeters) {

                vsyncIds.composite.emplace_back(id, targeter->target().vsyncId());

                results.try_emplace(id,

                                    CompositeResult{.compositionCoverage =

                                                            CompositionCoverage::Hwc});
@@ -486,21 +511,41 @@ TEST_F(SchedulerTest, onFrameSignalMultipleDisplays) { 
        }



        void sample() override {}

    } compositor;

    } compositor(*mScheduler);



    mScheduler->doFrameSignal(compositor, VsyncId(42));



    const auto makeVsyncIds = [](VsyncId vsyncId) -> VsyncIds {

    const auto makeVsyncIds = [](VsyncId vsyncId, bool swap = false) -> VsyncIds {

        if (swap) {

            return {{kDisplayId2, vsyncId}, {kDisplayId1, vsyncId}};

        } else {

            return {{kDisplayId1, vsyncId}, {kDisplayId2, vsyncId}};

        }

    };



    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds);

    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);

    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.composite);

    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds.commit);

    EXPECT_EQ(makeVsyncIds(VsyncId(42)), compositor.vsyncIds.composite);



    // FrameTargets should be updated despite the skipped commit.

    compositor.committed = false;

    mScheduler->doFrameSignal(compositor, VsyncId(43));



    // FrameTargets should be updated despite the skipped commit.

    EXPECT_EQ(makeVsyncIds(VsyncId(43)), compositor.vsyncIds);

    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);

    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.composite);

    EXPECT_EQ(makeVsyncIds(VsyncId(43)), compositor.vsyncIds.commit);

    EXPECT_TRUE(compositor.vsyncIds.composite.empty());



    // The pacesetter may change during commit.

    compositor.committed = true;

    compositor.changePacesetter = true;

    mScheduler->doFrameSignal(compositor, VsyncId(44));



    EXPECT_EQ(kDisplayId1, compositor.pacesetterIds.commit);

    EXPECT_EQ(kDisplayId2, compositor.pacesetterIds.composite);

    EXPECT_EQ(makeVsyncIds(VsyncId(44)), compositor.vsyncIds.commit);

    EXPECT_EQ(makeVsyncIds(VsyncId(44), true), compositor.vsyncIds.composite);

}



class AttachedChoreographerTest : public SchedulerTest {