rutabaga: do not rely on seals to detect read-only shm fds

[valpackett]

Some compositors send read-only but not sealed memfds (at least wlroots does so for dmabuf feedback), so seal-based permission detection would result in mapping RO memory as RW and the guest application crashing. Fix by properly checking the access mode. Keep seal detection because e.g. Smithay based compositors do send O_RDWR but write-sealed memfds.

Upstream rutabaga does not do any detection yet, but defaults to RO for all shm/memfd (!!) and RW for dmabuf.

---

I completely removed the seal part because it seems redundant (since seals are an extra facility that allows to make the RO-ness irreversible, so you shouldn't be able to have an RO seal on an RW descriptor) and upstream defaults to RO, but I wonder what the lore is behind checking for the seal and not for the flag…

I will propose proposed the checking for upstream rutabaga now too btw.

P.S. Please don't LLM-review this.

[gemini-code-assist]

Summary of Changes

Hello @valpackett, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the shared memory (ShmBlob) permission detection mechanism within the rutabaga graphics virtualization component. It moves away from relying on memfd seals, which were found to be unreliable for determining read-only status in certain scenarios, and instead directly checks the file descriptor's write permission flag. This change ensures that shared memory is mapped with the correct access rights, preventing potential guest application crashes due to incorrect memory access.

Highlights

• ShmBlob Permission Detection: Changed the logic for determining shared memory (ShmBlob) access permissions from checking F_SEAL_WRITE to inspecting the file descriptor's O_WRONLY flag.
• Preventing Guest Crashes: Addresses an issue where read-only but unsealed memfds were incorrectly mapped as read-write, leading to guest application crashes.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• src/rutabaga_gfx/src/cross_domain/mod.rs
  • Updated ShmBlob permission detection logic to use F_GETFL instead of F_GET_SEALS.
  • Changed default access for ShmBlob from read-write to read-only, adding write access only if explicitly writable.
  • Removed #[allow(unused_mut)] and #[cfg(target_os = "linux")] attributes related to the old seal-checking logic.

Activity

• No human activity has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request changes how read-only shared memory file descriptors are detected. It moves from checking for F_SEAL_WRITE to inspecting file access flags with fcntl. This is a good improvement to handle cases where memory is read-only but not sealed. However, the new implementation for checking write permission is flawed because it doesn't correctly handle file descriptors opened with O_RDWR. I've provided a suggestion to fix this by using the O_ACCMODE mask, which will correctly identify all writable descriptors.

[gemini-code-assist]

The check for write access is incorrect. flags & libc::O_WRONLY will be false if the file descriptor was opened with O_RDWR, as O_RDWR has a value of 2 while O_WRONLY is 1. This will cause a read-write descriptor to be incorrectly treated as read-only.

To correctly check for write permission, you should use the O_ACCMODE mask. A non-zero result from flags & O_ACCMODE indicates that the descriptor has write permissions (either O_WRONLY or O_RDWR), since O_RDONLY is 0.

I noticed this change makes the logic consistent with DmaBuf handling (lines 1158-1159), which appears to have the same bug. It would be beneficial to correct this in both places.

Suggested change

	if fcntl(descriptor.as_fd(), FcntlArg::F_GETFL)? & libc::O_WRONLY != 0 {
	if (fcntl(descriptor.as_fd(), FcntlArg::F_GETFL)? & libc::O_ACCMODE) != 0 {

[valpackett]

Whoops… the "correct" check makes it always crash, the "lore" is that I'm the one who added the seal check 🤦‍♀️ aaaand before that, the READ was changed to unconditional RW in the PR that implemented PipeWire support.

But it's not included in the PR currently upstreaming PipeWire support to rutabaga, so looks like that change was bogus all along (??)

---

UPD: now the actually correct check seems to work in all cases 🚀

[alyssais]

Hmm, I fixed something that was at least very similar to this upstream in 2023. https://chromium.googlesource.com/chromiumos/platform/crosvm/+/998597a1bd29432bdee28d298511549edff1434a%5E%21/