Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] misc: fastrpc: unbounded memory and DMA mapping leak via mismatched map flags - [High] misc: fastrpc: UAF of ext_cb array due to race with krealloc during probe - [High] fastrpc: refcount underflow via retained 0-refcount map on SCM error - [High] misc: fastrpc: memory leak and invalid hyp unmap on secure mapping failure - [Medium] misc: fastrpc: extended context bank mappings are bypassed and defeated during DSP invocation --
commit d890ee0bf9f03c55cbc6da25fffce9e57f51d755 Author: Vinayak Katoch <[email protected]> Subject: misc: fastrpc: add UAPI flags for extended IOVA mapping This commit introduces new UAPI flags, FASTRPC_MAP_FD_EXTENDED and FASTRPC_MAP_FD_DELAYED_EXTENDED, allowing userspace to map large buffers via the extended SMMU context bank. It adds a flags parameter to map creation and validation routines to handle the requested context bank type. > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d40cdcd5dec30..4ba07a5b28bb4 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -398,7 +404,7 @@ static void fastrpc_free_map(struct kref *ref) > err = qcom_scm_assign_mem(map->dma_addr, map->len, > &src_perms, &perm, 1); > if (err) { > - dev_err(map->fl->sctx->dev, > + dev_err(map->attach->dev, > "Failed to assign memory dma_addr %pad > size 0x%llx err %d\n", > &map->dma_addr, map->len, err); > return; [Severity: High] This problem wasn't introduced by this patch, but if qcom_scm_assign_mem() fails, the function returns early without unlinking the map from the list via list_del(). This leaves the map object in the fl->maps list with a 0-refcount. During session teardown, fastrpc_user_free() calls fastrpc_map_put() on all entries in fl->maps: drivers/misc/fastrpc.c:fastrpc_user_free() { list_for_each_entry_safe(map, m, &fl->maps, node) fastrpc_map_put(map); } Does the early return here strand the map object in the active list and lead to a double-put and refcount underflow? [ ... ] > @@ -956,7 +967,7 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, > int fd, > map->attr = attr; > err = qcom_scm_assign_mem(map->dma_addr, (u64)map->len, > &src_perms, dst_perms, 2); > if (err) { > - dev_err(sess->dev, > + dev_err(dev, > "Failed to assign memory with dma_addr %pad > size 0x%llx err %d\n", > &map->dma_addr, map->len, err); > goto get_err; [Severity: High] This problem wasn't introduced by this patch, but the map->attr field is set to the requested attributes before calling qcom_scm_assign_mem(). If this hypervisor call fails, the code jumps to get_err and calls fastrpc_map_put(). Because map->attr is already set to FASTRPC_ATTR_SECUREMAP, fastrpc_free_map() will attempt to transition the memory back to HLOS using a source permission mask that includes the secure VM: drivers/misc/fastrpc.c:fastrpc_free_map() { if (map->attr & FASTRPC_ATTR_SECUREMAP) { ... err = qcom_scm_assign_mem(map->dma_addr, map->len, &src_perms, &perm, 1); if (err) { dev_err(map->attach->dev, ...); return; } } } Since the initial transition never succeeded, the memory does not belong to the secure VM and this second qcom_scm_assign_mem() call will fail. Does setting map->attr before a successful hypervisor call lead to an invalid unmap attempt that returns early, leaking the DMA structures and map object? > } > @@ -979,13 +990,47 @@ static int fastrpc_map_attach(struct fastrpc_user *fl, > int fd, > return err; > } > > +static int fastrpc_map_attach(struct fastrpc_user *fl, int fd, > + u64 len, u32 attr, u32 flags, struct fastrpc_map > **ppmap) > +{ > + if (IS_EXTENDED_MAP_FLAG(flags)) { > + struct fastrpc_session_ctx **ext_cb; > + int i, count, err = -ENODEV; > + unsigned long lock_flags; > + > + spin_lock_irqsave(&fl->cctx->lock, lock_flags); > + count = fl->cctx->ext_cb_count; > + ext_cb = fl->cctx->ext_cb; > + spin_unlock_irqrestore(&fl->cctx->lock, lock_flags); > + > + if (!count) { > + dev_err(fl->sctx->dev, "no extended context bank > found\n"); > + return -ENODEV; > + } > + > + for (i = 0; i < count; i++) { > + err = fastrpc_map_attach_to_dev(fl, fd, len, attr, > flags, > + ext_cb[i], ppmap); [Severity: High] The ext_cb array pointer is cached under the lock but dereferenced outside of it in a loop. Because fastrpc_rpmsg_probe() registers the character device via fastrpc_device_register() before calling fastrpc_cb_devices_create(), userspace can concurrently issue a FASTRPC_IOCTL_MEM_MAP ioctl with an extended map flag. Concurrently, fastrpc_cb_init() resizes the array without RCU or synchronization against readers: drivers/misc/fastrpc.c:fastrpc_cb_init() { new_ext = krealloc(cctx->ext_cb, (cctx->ext_cb_count + 1) * sizeof(*cctx->ext_cb), GFP_KERNEL); ... spin_lock_irqsave(&cctx->lock, flags); cctx->ext_cb = new_ext; ... } If the memory is moved by krealloc(), it frees the old block. Can this lockless traversal of ext_cb race with krealloc() during device probe and cause a use-after-free? > + if (err != -ENOMEM) > + break; > + } > + return err; > + } > + > + return fastrpc_map_attach_to_dev(fl, fd, len, attr, flags, fl->sctx, > ppmap); > +} > + > static int fastrpc_map_create(struct fastrpc_user *fl, int fd, > - u64 len, u32 attr, struct fastrpc_map **ppmap) > + u64 len, u32 attr, u32 flags, struct fastrpc_map > **ppmap) > { > - if (!fastrpc_map_lookup(fl, fd, ppmap, true)) > - return 0; > + if (!fastrpc_map_lookup(fl, fd, ppmap, true)) { > + if ((*ppmap)->flags == flags) > + return 0; > + fastrpc_map_put(*ppmap); > + *ppmap = NULL; > + } [Severity: High] If a map with mismatched flags is found, its reference is dropped but it is not removed from the list. Since fastrpc_map_lookup() iterates from the head and always returns the first matching fd: drivers/misc/fastrpc.c:fastrpc_map_lookup() { list_for_each_entry(map, &fl->maps, node) { if (map->fd != fd || map->buf != buf) continue; ... *ppmap = map; ret = 0; break; } } Any subsequent mapping request for the same fd with different flags will repeatedly hit this old map, drop the newly acquired reference, and fall through to fastrpc_map_attach() where it appends a new map to the list. Could an unprivileged user exhaust kernel memory, DMA attachments, and SMMU IOVA space by alternating flags in repeated FASTRPC_IOCTL_MEM_MAP calls? > > - return fastrpc_map_attach(fl, fd, len, attr, ppmap); > + return fastrpc_map_attach(fl, fd, len, attr, flags, ppmap); > } > > /* > @@ -1063,10 +1108,10 @@ static int fastrpc_create_maps(struct > fastrpc_invoke_ctx *ctx) > > if (i < ctx->nbufs) > err = fastrpc_map_create(ctx->fl, ctx->args[i].fd, > - ctx->args[i].length, ctx->args[i].attr, > &ctx->maps[i]); > + ctx->args[i].length, ctx->args[i].attr, 0, > &ctx->maps[i]); [Severity: Medium] When processing argument buffers during a DSP invocation, fastrpc_map_create() is called with a hardcoded flags parameter of 0. If the user previously mapped a buffer to the extended CB, fastrpc_map_create() will find it but reject it because the cached flags do not match 0. The invocation path will then discard the extended map and create a fresh standard map attached to the regular SMMU CB, completely bypassing the wider IOVA window the user requested. Does hardcoding flags to 0 here cause valid extended-CB maps previously requested by the user to be defeated during DSP invocation? > else > err = fastrpc_map_attach(ctx->fl, ctx->args[i].fd, > - ctx->args[i].length, ctx->args[i].attr, > &ctx->maps[i]); > + ctx->args[i].length, ctx->args[i].attr, 0, > &ctx->maps[i]); > if (err) { > dev_err(dev, "Error Creating map %d\n", err); > return -EINVAL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
