Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free and premature SMMU unmapping due to missing reference counting for DMA handles. - [Critical] Data Race in `fastrpc_put_args()` leading to a Double Free of `fastrpc_map` objects. Pre-existing issues: - [High] Permanent Memory and IOMMU Leak of DMA handle maps on invocation error paths. -- commit de606379cd29bd912e7ec93a58384eaa07c8ecd6 Author: Jianping Li <[email protected]> misc: fastrpc: avoid duplicate DMA mappings in fastrpc_create_maps() This patch attempts to fix duplicate DMA mappings by not taking an extra reference for DMA handle arguments (take_ref = false). It tags them with FASTRPC_MAP_DMA_HANDLE, re-validates the map in fastrpc_get_args() before use, and relies on fastrpc_put_args() to release these tagged maps when the DSP returns the fd. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c > index d4fac2caca867..58f27e2713172 100644 > --- a/drivers/misc/fastrpc.c > +++ b/drivers/misc/fastrpc.c [ ... ] > @@ -1047,23 +1052,25 @@ static int fastrpc_create_maps(struct > fastrpc_invoke_ctx *ctx) > int i, err; > > for (i = 0; i < ctx->nscalars; ++i) { > + bool take_ref = i < ctx->nbufs; > + int mflags = 0; > > if (ctx->args[i].fd == 0 || ctx->args[i].fd == -1 || > ctx->args[i].length == 0) > continue; > > - 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]); > - else > - err = fastrpc_map_attach(ctx->fl, ctx->args[i].fd, > - ctx->args[i].length, ctx->args[i].attr, > &ctx->maps[i]); > + /* Set the DMA handle mapping flag for DMA handles */ > + if (i >= ctx->nbufs) > + mflags = FASTRPC_MAP_DMA_HANDLE; > + > + err = fastrpc_map_create(ctx->fl, ctx->args[i].fd, > ctx->args[i].length, > + ctx->args[i].attr, &ctx->maps[i], > take_ref, mflags); [Severity: High] This is a pre-existing issue, but does fastrpc_create_maps() leak the DMA handle maps on error paths? If this function successfully creates a DMA handle map (where i >= ctx->nbufs), but a subsequent step in fastrpc_internal_invoke() fails, the cleanup path in fastrpc_context_free() is triggered. fastrpc_context_free() only iterates up to nbufs: for (i = 0; i < ctx->nbufs; i++) fastrpc_map_put(ctx->maps[i]); Since the cleanup loop ignores indices >= nbufs, and the invoke never reaches the DSP to trigger fastrpc_put_args(), do these maps and their SMMU mappings leak permanently? > if (err) { > dev_err(dev, "Error Creating map %d\n", err); > return -EINVAL; > } > - > } > + > return 0; > } [ ... ] > @@ -1195,6 +1202,16 @@ static int fastrpc_get_args(u32 kernel, struct > fastrpc_invoke_ctx *ctx) > list[i].num = ctx->args[i].length ? 1 : 0; > list[i].pgidx = i; > if (ctx->maps[i]) { > + /* It is possible that map is created with > + * mflags FASTRPC_MAP_DMA_HANDLE and take_ref > + * is false. Check if map still exists or is > + * being freed as take_ref is false > + */ > + if (fastrpc_map_lookup(ctx->fl, ctx->args[i].fd, > + &ctx->maps[i], false)) { > + ctx->maps[i] = NULL; > + return -EINVAL; > + } > pages[i].addr = ctx->maps[i]->dma_addr; [Severity: Critical] Can this lockless dereference lead to a use-after-free regression? Since fastrpc_create_maps() maps the DMA handle with take_ref = false, the shared map's refcount remains 1. If a concurrent invocation completes and executes fastrpc_put_args() exactly after fastrpc_map_lookup() drops the lock but before ctx->maps[i]->dma_addr is evaluated, it will clear the flag and free the map. Would this result in a host kernel use-after-free and premature SMMU unmapping when the unreferenced pointer is accessed? > pages[i].size = ctx->maps[i]->size; > } [ ... ] > @@ -1244,8 +1261,17 @@ static int fastrpc_put_args(struct fastrpc_invoke_ctx > *ctx, > for (i = 0; i < FASTRPC_MAX_FDLIST; i++) { > if (!fdlist[i]) > break; > - if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false)) > + /* > + * DMA handle maps are released when the DSP returns the > corresponding fd in > + * fdlist. The DSP is expected to return a specific fd only > once in fdlist, > + * so no two fastrpc_put_args() paths should clear the > DMA_HANDLE flag for > + * the same map concurrently. > + */ > + if (!fastrpc_map_lookup(fl, (int)fdlist[i], &mmap, false) && > + mmap->flags == FASTRPC_MAP_DMA_HANDLE) { > + mmap->flags = 0; > fastrpc_map_put(mmap); > + } [Severity: Critical] Does clearing mmap->flags outside a lock introduce a race condition leading to a double free regression? If userspace submits two concurrent invocations sharing the same fd, and the DSP returns the fd in the fdlist of both, two threads executing fastrpc_put_args() could look up the shared map locklessly. Thread 1: evaluates mmap->flags == FASTRPC_MAP_DMA_HANDLE as true Thread 2: evaluates mmap->flags == FASTRPC_MAP_DMA_HANDLE as true Thread 1: clears flags and calls fastrpc_map_put() freeing the map Thread 2: clears flags and calls fastrpc_map_put() on a freed pointer Can this lead to host kernel heap corruption if triggered by concurrent invokes? > } > > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
