linux-kernel - Re: [PATCH v1 2/3] misc: fastrpc: Fix fastrpc_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <qeoclfg7inqwdzbxl35wsu6z7xecbc77albmnbge3h3hfr5uph@wxl5vfjfs35u>
Date: Fri, 30 Aug 2024 12:31:52 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>
Cc: srinivas.kandagatla@...aro.org, linux-arm-msm@...r.kernel.org, 
	gregkh@...uxfoundation.org, quic_bkumar@...cinc.com, linux-kernel@...r.kernel.org, 
	quic_chennak@...cinc.com, dri-devel@...ts.freedesktop.org, arnd@...db.de, 
	stable <stable@...nel.org>
Subject: Re: [PATCH v1 2/3] misc: fastrpc: Fix fastrpc_map_lookup operation

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@...nel.org>
> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
> ---
>  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