VOOZH about

URL: https://phabricator.services.mozilla.com/D290523

⇱ ⚙ D290523 Bug 2021722 - Sandbox - RDD sandbox policy for Vulkan video. r?stransky

Paths

Bug 2021722 - Sandbox - RDD sandbox policy for Vulkan video. r?stransky
ClosedPublic

Authored by tboiko on Mar 27 2026, 4:48 PM.
Referenced Files
Unknown Object (File)
Fri, Jun 12, 6:13 AM
Unknown Object (File)
Thu, Jun 11, 8:25 PM
Unknown Object (File)
Wed, Jun 3, 1:30 AM
Unknown Object (File)
Tue, Jun 2, 12:32 PM
Unknown Object (File)
Tue, Jun 2, 9:28 AM
Unknown Object (File)
Tue, Jun 2, 9:16 AM
Unknown Object (File)
Tue, Jun 2, 8:26 AM
Unknown Object (File)
Tue, Jun 2, 8:11 AM

Details

Summary

Sandbox:

  • Allow required syscalls and broker paths for Vulkan video in the RDD process (seccomp and filesystem broker policy updates for drivers, sockets, and related resources).

Diff Detail

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
Comment Actions

The analysis task source-test-clang-tidy failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-format failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-external failed, but we could not detect any defect.
Please check this task manually.

If you see a problem in this automated review, please report it here.

Comment Actions

The analysis task source-test-clang-external failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-tidy failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-format failed, but we could not detect any defect.
Please check this task manually.

If you see a problem in this automated review, please report it here.

Comment Actions

OK, added /usr/share/vulkan/icd.d to AddVulkanDependencies() as well

Comment Actions

The analysis task source-test-clang-external failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-tidy failed, but we could not detect any defect.
Please check this task manually.

The analysis task source-test-clang-format failed, but we could not detect any defect.
Please check this task manually.

If you see a problem in this automated review, please report it here.

Comment Actions

Code analysis found 6 defects in diff 1257428:

  • 6 build errors found by clang-tidy
IMPORTANT: Found 6 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

  • (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1257428.

Comment Actions

Code analysis found 6 defects in diff 1262357:

  • 6 build errors found by clang-tidy
IMPORTANT: Found 6 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

  • (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1262357.

Comment Actions

Code analysis found 6 defects in diff 1267103:

  • 6 build errors found by clang-tidy
IMPORTANT: Found 6 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

  • (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1267103.

Comment Actions

@jld , can you please take a look? I have fixed all the comments as above

jld requested changes to this revision.Thu, May 21, 5:27 AM
Comment Actions

I'm concerned about the security implications of the rules. Hopefully there's a way to reduce those to something safer. If that's not possible, then at a minimum I'd like to make those rules conditional on detecting the hardware in question.

security/sandbox/linux/SandboxFilter.cpp
2154

should be handled in as .

But also, why do we need this? This patch isn't allowing or , so I assume this would only be for setting the source address on an outgoing connection, but I don't think that will work with the brokered (it receives a new socket from the broker and s over the old one).

For the same reason, it's probably harmless to allow , but I wonder if this is really doing what it was meant to.

2160–2162

I believe won't ever be null, so this should be deleted. (Also, allowing and unconditionally would be a significant hole in the sandbox.)

security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
917–928

This seems to be dead code?

932–935

This adds a number of sandbox escapes. The X11 server is the most obvious one, but I believe unrestricted DBus access is also a sandbox escape, and that rule also includes PulseAudio (accepts commands to arbitrary files), , etc., various other things that probably aren't expecting hostile input from a same-uid client.

Is it really necessary to allow all of this, given that VA-API didn't need it? Would it be possible to narrow these rules and/or move some of the initialization before starting the sandbox, to avoid needing to do this?

This revision now requires changes to proceed.Thu, May 21, 5:27 AM
tboiko updated this revision to Diff 1279744.
Comment Actions

Done, the vulkan decoding works with these changes at my side.

security/sandbox/linux/SandboxFilter.cpp
2154

removed this. it works without it.

2160–2162

removed this. it works without it

security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
917–928

removed this. yes, this code is unused

932–935

removed this. it works without it

Comment Actions

OK, updated

security/sandbox/linux/SandboxFilter.cpp
2154

@jld, actually, I remember @stransky reported a seccomp sandbox violation without "bind":
https://phabricator.services.mozilla.com/D286552#10063337
So, it would be better to keep it in EvaluateSocketCall as SYS_BIND instead of removing, like you suggested

Comment Actions

r+, modulo some minor comments.

Testing note: I assume that existing video tests will provide coverage for this if run on a suitable system. (In general this is kind of difficult for CI because of needing specific real hardware; if I recall correctly Mozilla CI has some coverage for WebGL but not for VA-API support, although that may have changed since I last tried to look into it.)

security/sandbox/linux/SandboxFilter.cpp
1975

If there's no more , you should be able to drop the as well.

2022

You could see if this works with or something like that; if not, the is fine as previously discussed.

2027–2029

See above re .

This revision is now accepted and ready to land.Thu, Jun 4, 5:13 AM
Comment Actions

OK, updated as per the minor comments above
Thank you for the review

security/sandbox/linux/SandboxFilter.cpp
1975

OK, removed

2022

Modified as above, works for me

2027–2029

OK, removed

Revision Contents

PathSize
security/
sandbox/
linux/
34 lines
broker/
56 lines
CommitTreeParentsAuthorSummaryDate
e5f8199fc24797837a4fbb1bTymur Boiko
Bug 2021722 - Sandbox - RDD sandbox policy for Vulkan video. r=jld (Show More…)

Diff 1292039

security/sandbox/linux/SandboxFilter.cpp

Loading...

security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp

Loading...