Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

service/ldr_ro: Fix CRO loading when the buffer contained multiple VM areas #5125

Merged
merged 2 commits into from Mar 28, 2020

Conversation

zhaowenlan1779
Copy link
Member

@zhaowenlan1779 zhaowenlan1779 commented Mar 10, 2020

Fixes #4493

Fixed two bugs regarding adjacent memory areas with the same permission and state, but different backing pointers.


According to wwylele, the reason the bug in RO was made was that

  • on 3DS the ro module uses svcQueryProcessMemory to retrieve the vma info and do the same four checks as coded in VerifyBufferState
  • But later we found out that svcQueryProcessMemory would actually merge similar adjacent areas before reporting.
    // Query(Process)Memory merges vma with neighbours when they share the same state and
    // permissions, regardless of their physical mapping.
    auto mismatch = [permissions, state](const std::pair<VAddr, Kernel::VirtualMemoryArea>& v) {
    return v.second.permissions != permissions || v.second.meminfo_state != state;
    };
    std::reverse_iterator rvma(vma);
    auto lower = std::find_if(rvma, process->vm_manager.vma_map.crend(), mismatch);
    --lower;
    auto upper = std::find_if(vma, process->vm_manager.vma_map.cend(), mismatch);
    --upper;

so CRO code should contain the same merging logic or equivalent, or directly invoke the SVC implementation.


This change is Reviewable

It is possible that a few areas have the same permisson and state, but with different backing pointers. Currently, this function assumes that only one continous area is found, but this is not always the case.
It is possible that the buffer passed from the game is made up of multiple areas with the same permisson and state but different backing pointers. Change the check to allow that.
B3n30
B3n30 approved these changes Mar 10, 2020
@@ -151,13 +151,16 @@ ResultCode VMManager::ChangeMemoryState(VAddr target, u32 size, MemoryState expe
}

CASCADE_RESULT(auto vma, CarveVMARange(target, size));
ASSERT(vma->second.size == size);
Copy link
Contributor

@B3n30 B3n30 Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't there be still an assert for the overall size?

Copy link
Member Author

@zhaowenlan1779 zhaowenlan1779 Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is actually not necessary, as CarveVMARange will take care of this

@B3n30 B3n30 merged commit 3edc4a3 into citra-emu:master Mar 28, 2020
2 of 3 checks passed
@zhaowenlan1779 zhaowenlan1779 deleted the vma-cro-fix branch May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants