From 1dbd9838c438b6b2a9eb8e2f6e0b1ec8da02b578 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Wed, 26 Aug 2026 17:44:39 +0100 Subject: [PATCH 1/2] staging: vc04_services: vchiq-mmal: Bound-check the parameter payload 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 --- drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c index ec42af0ed45a0f..245cfec3ef3700 100644 --- a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c +++ b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c @@ -1349,6 +1349,9 @@ static int port_parameter_set(struct vchiq_mmal_instance *instance, struct mmal_msg *rmsg; struct vchiq_header *rmsg_handle; + if (value_size > sizeof(m.u.port_parameter_set.value)) + return -EINVAL; + m.h.type = MMAL_MSG_TYPE_PORT_PARAMETER_SET; m.u.port_parameter_set.component_handle = port->component->handle; @@ -1390,6 +1393,9 @@ static int port_parameter_get(struct vchiq_mmal_instance *instance, struct mmal_msg *rmsg; struct vchiq_header *rmsg_handle; + if (*value_size > sizeof(rmsg->u.port_parameter_get_reply.value)) + return -EINVAL; + m.h.type = MMAL_MSG_TYPE_PORT_PARAMETER_GET; m.u.port_parameter_get.component_handle = port->component->handle; From 4b8a9059ae3d547972842e32489c8f18b52347c7 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Wed, 26 Aug 2026 17:51:38 +0100 Subject: [PATCH 2/2] staging: vc04_services: vc-sm-cma: Fix fd handling when copy_to_user 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 --- drivers/staging/vc04_services/vc-sm-cma/vc_sm.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c b/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c index b0aff3974f24f6..3d4e2232e353e4 100644 --- a/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c +++ b/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -1200,8 +1201,10 @@ static int vc_sm_cma_ioctl_alloc(struct vc_sm_privdata_t *private, buffer->alloc.sg_table = sgt; fd = dma_buf_fd(dmabuf, O_CLOEXEC); - if (fd < 0) + if (fd < 0) { + ret = fd; goto error; + } vc_sm_add_resource(private, buffer); @@ -1276,9 +1279,9 @@ static long vc_sm_cma_ioctl(struct file *file, unsigned int cmd, if (!ret && (copy_to_user((void *)arg, &ioparam, sizeof(ioparam)) != 0)) { - /* FIXME: Release allocation */ pr_err("[%s]: failed to copy-to-user for cmd %x\n", __func__, cmdnr); + close_fd(ioparam.handle); ret = -EFAULT; } break; @@ -1312,11 +1315,12 @@ static long vc_sm_cma_ioctl(struct file *file, unsigned int cmd, ioparam.vc_handle = buf->vc_handle; ioparam.dma_addr = buf->dma_addr; - if (ioparam.handle < 0 || - (copy_to_user((void *)arg, &ioparam, - sizeof(ioparam)) != 0)) { + if (ioparam.handle < 0) { dma_buf_put(new_dmabuf); - /* FIXME: Release allocation */ + ret = -EFAULT; + } else if (copy_to_user((void *)arg, &ioparam, + sizeof(ioparam)) != 0) { + close_fd(ioparam.handle); ret = -EFAULT; } }