vc04_services: bounds and fd handling fixes - #7576
Draft
popcornmix wants to merge 2 commits into
Draft
Conversation
… size struct mmal_msg is a 512-byte stack buffer: a 24-byte header plus a 488-byte union. Within that union, mmal_msg_port_parameter_set places its 384-byte value[] at offset 16, so the field ends 88 bytes short of the end of the union. port_parameter_set() copies value_size bytes into that field without checking it against the field size. A value_size of 385..472 corrupts the rest of the union, and anything above 472 runs off the end of the on-stack struct mmal_msg. send_synchronous_mmal_msg() does reject a payload longer than the union can hold, which covers exactly the second case, but it is only reached after the memcpy has already happened. port_parameter_get() has the mirror-image problem: *value_size is both advertised to the firmware as the space available and used to bound the copy out of the reply, so a caller asking for more than 384 bytes reads past value[] in the received message and into whatever follows it in the VCHIQ slot. No in-tree caller can trigger either: every call site passes a sizeof() of a fixed structure, the largest being the 280-byte encoding list read by bcm2835_isp_get_supported_fmts(). Both functions are reachable through exported symbols though, so reject an oversized size rather than relying on callers to get it right. Signed-off-by: Dom Cobley <popcornmix@gmail.com>
…fails Both VC_SM_CMA_CMD_ALLOC and VC_SM_CMA_CMD_IMPORT_DMABUF install a dmabuf fd into the caller's fd table before copying the result struct back out. If that copy_to_user() fails the ioctl returns -EFAULT, but the fd stays installed and userspace never learns its number, so it cannot close it. For ALLOC that leaves the CMA allocation, the vc_sm_buffer and the VPU mapping pinned until the process exits, and lets a caller pin an fd per ioctl up to RLIMIT_NOFILE. Release the fd with close_fd() instead. IMPORT is worse. dma_buf_fd() does not take a reference of its own, it transfers the caller's reference to the fd table. Once it has succeeded the dma_buf_put() on the copy_to_user() error path drops a reference the ioctl no longer owns, freeing the dmabuf while the installed fd still points at it - a refcount underflow and a use-after-free on the subsequent close. Split the two conditions so dma_buf_put() only runs when dma_buf_fd() actually failed, and use close_fd() otherwise. While here, propagate the dma_buf_fd() error in vc_sm_cma_ioctl_alloc() rather than falling into the error path with ret still 0, which made the ioctl report success with an uninitialised handle. The device is registered with mode 0666, so this is reachable by any local user. Signed-off-by: Dom Cobley <popcornmix@gmail.com>
6by9
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.