On Thu, Aug 22, 2024 at 04:29:32PM GMT, Ekansh Gupta wrote:
> Fastrpc driver creates maps for user allocated fd buffers. Before
> creating a new map, the map list is checked for any already existing
> maps using map fd. Checking with just map fd is not sufficient as the
> user can pass offsetted buffer with less size when the map is created
> and then a larger size the next time which could result in memory
> issues. Check for user passed VA and length also when looking up for
> the map.
>
> Fixes: c68cfb718c8f ("misc: fastrpc: Add support for context Invoke method")
> Cc: stable <stable@xxxxxxxxxx>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@xxxxxxxxxxx>
> ---
> drivers/misc/fastrpc.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index bcfb8ce1a0e3..ebe828770a8d 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -362,7 +362,8 @@ static int fastrpc_map_get(struct fastrpc_map *map)
>
>
> static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
> - struct fastrpc_map **ppmap, bool take_ref)
> + u64 va, u64 len, struct fastrpc_map **ppmap,
> + bool take_ref)
Please use consistent alignment between the lines.
> {
> struct fastrpc_session_ctx *sess = fl->sctx;
> struct fastrpc_map *map = NULL;
> @@ -370,7 +371,8 @@ static int fastrpc_map_lookup(struct fastrpc_user *fl, int fd,
>
> spin_lock(&fl->lock);
> list_for_each_entry(map, &fl->maps, node) {
> - if (map->fd != fd)
> + if (map->fd != fd || va < (u64)map->va ||
> + va + len > (u64)map->va + map->size)
> continue;
>
> if (take_ref) {
> @@ -752,7 +754,8 @@ static const struct dma_buf_ops fastrpc_dma_buf_ops = {
> };
>
> static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> - u64 len, u32 attr, struct fastrpc_map **ppmap)
> + u64 va, u64 len, u32 attr,
> + struct fastrpc_map **ppmap)
> {
> struct fastrpc_session_ctx *sess = fl->sctx;
> struct fastrpc_map *map = NULL;
> @@ -760,7 +763,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> struct scatterlist *sgl = NULL;
> int err = 0, sgl_index = 0;
>
> - if (!fastrpc_map_lookup(fl, fd, ppmap, true))
> + if (!fastrpc_map_lookup(fl, fd, va, len, ppmap, true))
> return 0;
>
> map = kzalloc(sizeof(*map), GFP_KERNEL);
> @@ -807,7 +810,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
> err = -EINVAL;
> goto map_err;
> }
> - map->va = sg_virt(map->table->sgl);
> + map->va = (void *)(uintptr_t)va;
This looks unrelated
> map->len = len;
>
> if (attr & FASTRPC_ATTR_SECUREMAP) {
> @@ -920,7 +923,8 @@ static int fastrpc_create_maps(struct fastrpc_invoke_ctx *ctx)
> continue;
>
> err = fastrpc_map_create(ctx->fl, ctx->args[i].fd,
> - ctx->args[i].length, ctx->args[i].attr, &ctx->maps[i]);
> + (u64)ctx->args[i].ptr, ctx->args[i].length,
> + ctx->args[i].attr, &ctx->maps[i]);
> if (err) {
> dev_err(dev, "Error Creating map %d\n", err);
> return -EINVAL;
> @@ -1106,7 +1110,7 @@ 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))
> + if (!fastrpc_map_lookup(fl, (int)fdlist[i], 0, 0, &mmap, false))
Isn't this going to always return false?
> fastrpc_map_put(mmap);
> }
>
> @@ -1412,7 +1416,8 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
> fl->pd = USER_PD;
>
> if (init.filelen && init.filefd) {
> - err = fastrpc_map_create(fl, init.filefd, init.filelen, 0, &map);
> + err = fastrpc_map_create(fl, init.filefd, init.file,
> + init.filelen, 0, &map);
> if (err)
> goto err;
> }
> @@ -2034,7 +2039,8 @@ static int fastrpc_req_mem_map(struct fastrpc_user *fl, char __user *argp)
> return -EFAULT;
>
> /* create SMMU mapping */
> - err = fastrpc_map_create(fl, req.fd, req.length, 0, &map);
> + err = fastrpc_map_create(fl, req.fd, req.vaddrin, req.length,
> + 0, &map);
> if (err) {
> dev_err(dev, "failed to map buffer, fd = %d\n", req.fd);
> return err;
> --
> 2.34.1
>
--
With best wishes
Dmitry
