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

msgq: fix permission of subscribers #636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

katsuster
Copy link

The current implementation of MSGQ and VisionIPC allows subscriber processes to potentially overwrite publisher's data because subscribers have write permissions for shared data area of the queue.

This patch changes mmap() protection of subscribers from PROT_READ | PROT_WRITE to PROT_READ only.
This prevents unintended writes by subscribers and ensures the integrity of publisher's data.

This patch changes mmap() protection of subscribers from 'PROT_READ |
PROT_WRITE' to 'PROT_READ' only.

It can prevent data area curruption by unintentional writes from
subscribers to shared memory area.
This patch fixes tests of msgq to distinguish between
msgq_new_queue() funtions of pub/sub.
@adeebshihadeh
Copy link
Contributor

Seems like it's still easy to setup a subscriber that can write. Can you add a test to ensure that doesn't happen? Or even better, can you think of a way to rewrite it to make that bug less likely?

@katsuster
Copy link
Author

Thank you for your comment!
It's OK that subscribers can setup write permission easily. I don't intend to ensure protecting from 'malicious subscribers' that try to get write permissions and destroy publisher's data intentionally.

I'm targeting for 'normal subscribers' that use PubMaster/SubMaster API cannot take unneeded write permissions.
I also want to prevent silent data corruption by unintentional write to shared area due to their implementation bugs.

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.

2 participants