Re: [PATCH v2 4/4] misc: fastrpc: add UAPI flags for extended IOVA mapping

[sashiko-bot@kernel.org]

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 <vinayak.katoch@oss.qualcomm.com>
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/20261007-extended-mapping-v2-0-5dca3aa2c2d3@oss.qualcomm.com?part=4