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: <e9c354d2-7b26-4573-a415-b2b82ac1291b@quicinc.com>
Date: Thu, 19 Sep 2024 11:45:45 +0530
From: Ekansh Gupta <quic_ekangupt@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
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 8/30/2024 3:01 PM, Dmitry Baryshkov wrote:
> 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.
Ack.
>>  {
>>  	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
Will be dropping this.
>
>>  	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?
Yes, I'll correct this.
>
>>  			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
>>