Re: [PATCH] misc: fastrpc: exclude mapped buffers from inline overlaps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer truncation in `__fastrpc_buf_alloc` allows out-of-bounds DMA access.
- [Critical] Use-After-Free and Double Free in `fastrpc_map_attach` error path.
- [Critical] Integer truncation of `mlen` allows kernel buffer overflow via `copy_from_user`.
- [High] Memory leak of `struct fastrpc_map` for handles during context cleanup.
- [High] Circular reference leak when `fastrpc_internal_invoke` is interrupted.
--

commit a1f9ba556dd9f43a1a5e5fbad752e8ca90d62449
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
Subject: misc: fastrpc: exclude mapped buffers from inline overlaps

This commit modifies fastrpc_get_buff_overlaps() to ignore mapped buffers when
computing overlap offsets. It ensures that only buffers without a file
descriptor are serialized into the inline DMA payload. This prevents the
serializer from deriving negative offsets and writing to unreserved memory.

> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index f3a49384586d1..79b428addf062 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does passing the 64-bit buf->size to the 
size_t argument of dma_alloc_coherent() result in truncation on 32-bit 
architectures?

drivers/misc/fastrpc.c:__fastrpc_buf_alloc() {
    ...
    buf->virt = dma_alloc_coherent(dev, buf->size, &buf->dma_addr, GFP_KERNEL);
    ...
}

Could this cause an out-of-bounds DMA access when the untruncated 64-bit size 
is sent to the DSP here?

drivers/misc/fastrpc.c:fastrpc_invoke_send() {
    ...
    msg->size = roundup(ctx->msg_sz, PAGE_SIZE);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this allocation lack a corresponding 
cleanup path for handles?

drivers/misc/fastrpc.c:fastrpc_create_maps() {
    ...
    else
        err = fastrpc_map_attach(ctx->fl, ctx->args[i].fd, ctx->args[i].length, ctx->args[i].attr, &ctx->maps[i]);
    ...
}

The cleanup loop in fastrpc_context_free() terminates prematurely at ctx->nbufs:

drivers/misc/fastrpc.c:fastrpc_context_free() {
    ...
    for (i = 0; i < ctx->nbufs; i++)
        fastrpc_map_put(ctx->maps[i]);
    ...
}

Does this fail to drop references for mappings populated up to ctx->nscalars, 
resulting in a memory leak of struct fastrpc_map?

[Severity: High]
This is a pre-existing issue, but does this create a circular reference if the 
context is not freed?

drivers/misc/fastrpc.c:fastrpc_context_alloc() {
    ...
    /* Take a reference to user, released in fastrpc_context_free() */
    fastrpc_user_get(user);
    ...
}

Looking at the error path for -ERESTARTSYS, it bypasses context cleanup:

drivers/misc/fastrpc.c:fastrpc_internal_invoke() {
    ...
    if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
        spin_lock(&fl->lock);
        list_del(&ctx->node);
        spin_unlock(&fl->lock);
        fastrpc_context_put(ctx);
    }
    ...
}

If interrupted, does this strand the context with its elevated refcount and leak 
the fastrpc_invoke_ctx and fastrpc_user resources?

[Severity: Critical]
This is a pre-existing issue, but the error path manually detaches and puts the 
buffer, then drops the map reference:

drivers/misc/fastrpc.c:fastrpc_map_attach() {
    ...
map_err:
    dma_buf_detach(map->buf, map->attach);
attach_err:
    dma_buf_put(map->buf);
get_err:
    fastrpc_map_put(map);
    ...
}

Does fastrpc_free_map() then execute its own cleanup block because map->table 
is non-NULL?

drivers/misc/fastrpc.c:fastrpc_free_map() {
    ...
    if (map->table) {
        // ...
        dma_buf_unmap_attachment_unlocked(map->attach, map->table, DMA_BIDIRECTIONAL);
        dma_buf_detach(map->buf, map->attach);
        dma_buf_put(map->buf);
    }
    ...
}

Could this lead to a use-after-free and double free of the dma_buf_attachment?

[Severity: Critical]
This is a pre-existing issue, but does assigning the 64-bit difference to the 
32-bit int mlen silently truncate the value?

drivers/misc/fastrpc.c:fastrpc_get_args() {
    ...
    for (oix = 0; oix < ctx->nbufs; ++oix) {
        int mlen;
        // ...
        mlen = ctx->olaps[oix].mend - ctx->olaps[oix].mstart;
    ...
}

Can this truncation cause validation checks to fail and pointer arithmetic to 
underflow, allowing an out-of-bounds write during copy_from_user?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624192309.4723-1-alhouseenyousef@gmail.com?part=1