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

machine/spi_sdcard.cpp: fix write data leaking into cmd buffer #12249

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

holub
Copy link
Contributor

@holub holub commented Apr 14, 2024

somehow related to MT08855 - but it's unclear how "wrong" spi commands can kill entire emu memory.

@cuavas
Copy link
Member

cuavas commented Apr 15, 2024

This really looks like a “fix the symptom rather than the cause” change. Is the software using CMD16 to set a block size that’s too large for your fixed-size buffer m_data? Is something corrupting m_write_ptr?

Have you tried logging m_write_ptr after each state change to ensure it isn’t getting corrupted?

@holub
Copy link
Contributor Author

holub commented Apr 15, 2024

This really looks like a “fix the symptom rather than the cause” change. Is the software using CMD16 to set a block size that’s too large for your fixed-size buffer m_data? Is something corrupting m_write_ptr?

Have you tried logging m_write_ptr after each state change to ensure it isn’t getting corrupted?

The problem is different here: software writes right amount of data and this data is stored. But because this data is simultaneously written to cmd buffer, the tail of data is treated as a command and processed during the next cycle.

@holub
Copy link
Contributor Author

holub commented Apr 16, 2024

I insist that's the actual fix.
Similar to the situation we do clean_cmd after each command to clean m_cmd buffer we need to do it here. The only difference is that command happens before we start receiving the m_data. As a result after data is received m_cmd is polluted with the tail of data received and this is treated as a next command which shouldn't be a case.

... I had also tried to assert writes beyond m_data and never catched such

@pmackinlay
Copy link
Contributor

Surely the real hardware knows when it's expecting data (rather than commands), and shouldn't be adding data into the command buffer in the first place? Can you make the command buffer byte latching conditional on the state to avoid having to "clean" it in either case?

@holub
Copy link
Contributor Author

holub commented Apr 16, 2024

I did minimal refactoring which can be considered as a better fix

@cuavas cuavas merged commit 407783c into mamedev:master Apr 16, 2024
5 checks passed
@holub holub deleted the spi branch April 16, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants