[PATCH 0/3] fastrpc: fix dmabuf arguments

[PATCH 0/3] fastrpc: fix dmabuf arguments

[Jonathan Marek]

dmabuf arguments are completely broken, these patches make it possible
to actually use dmabuf arguments (in particular, the second patch).

Jonathan Marek (3):
  fastrpc: always use fl->lock and remove fl->mutex
  fastrpc: move fl->maps list removal into fastrpc_free_map
  fastrpc: remove redundant fastrpc_map_create() call

 drivers/misc/fastrpc.c | 48 ++++++++++++++----------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

-- 
2.26.1

[PATCH 1/3] fastrpc: always use fl->lock and remove fl->mutex

[Jonathan Marek]

Fix the broken behavior of having a separate mutex for locking at this
place, and use the same spinlock that is used to add/remove from the list.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/misc/fastrpc.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index f12e909034ac..4fabea0c1551 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -230,8 +230,6 @@ struct fastrpc_user {
 	int pd;
 	/* Lock for lists */
 	spinlock_t lock;
-	/* lock for allocations */
-	struct mutex mutex;
 };
 
 static void fastrpc_free_map(struct kref *ref)
@@ -267,16 +265,16 @@ static int fastrpc_map_find(struct fastrpc_user *fl, int fd,
 {
 	struct fastrpc_map *map = NULL;
 
-	mutex_lock(&fl->mutex);
+	spin_lock(&fl->lock);
 	list_for_each_entry(map, &fl->maps, node) {
 		if (map->fd == fd) {
 			fastrpc_map_get(map);
 			*ppmap = map;
-			mutex_unlock(&fl->mutex);
+			spin_unlock(&fl->lock);
 			return 0;
 		}
 	}
-	mutex_unlock(&fl->mutex);
+	spin_unlock(&fl->lock);
 
 	return -ENOENT;
 }
@@ -1200,7 +1198,6 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 	fastrpc_session_free(cctx, fl->sctx);
 	fastrpc_channel_ctx_put(cctx);
 
-	mutex_destroy(&fl->mutex);
 	kfree(fl);
 	file->private_data = NULL;
 
@@ -1222,7 +1219,6 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 
 	filp->private_data = fl;
 	spin_lock_init(&fl->lock);
-	mutex_init(&fl->mutex);
 	INIT_LIST_HEAD(&fl->pending);
 	INIT_LIST_HEAD(&fl->maps);
 	INIT_LIST_HEAD(&fl->mmaps);
@@ -1233,7 +1229,6 @@ static int fastrpc_device_open(struct inode *inode, struct file *filp)
 	fl->sctx = fastrpc_session_alloc(cctx);
 	if (!fl->sctx) {
 		dev_err(&cctx->rpdev->dev, "No session available\n");
-		mutex_destroy(&fl->mutex);
 		kfree(fl);
 
 		return -EBUSY;
-- 
2.26.1

[PATCH 2/3] fastrpc: move fl->maps list removal into fastrpc_free_map

[Jonathan Marek]

This fixes the incredibly broken behavior of fastrpc_context_free(),
which calls fastrpc_map_put() but does not remove the map from the list
when it is free'd, causing use-after-free errors.

fl->lock needs to be held not just for list_del(), but also kref_put, to
avoid a race condition with fastrpc_map_find() logic.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/misc/fastrpc.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 4fabea0c1551..170352b43ab6 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -245,19 +245,21 @@ static void fastrpc_free_map(struct kref *ref)
 		dma_buf_put(map->buf);
 	}
 
+	list_del(&map->node);
+
 	kfree(map);
 }
 
 static void fastrpc_map_put(struct fastrpc_map *map)
 {
-	if (map)
-		kref_put(&map->refcount, fastrpc_free_map);
+	spin_lock(&map->fl->lock);
+	kref_put(&map->refcount, fastrpc_free_map);
+	spin_unlock(&map->fl->lock);
 }
 
 static void fastrpc_map_get(struct fastrpc_map *map)
 {
-	if (map)
-		kref_get(&map->refcount);
+	kref_get(&map->refcount);
 }
 
 static int fastrpc_map_find(struct fastrpc_user *fl, int fd,
@@ -351,8 +353,10 @@ static void fastrpc_context_free(struct kref *ref)
 	ctx = container_of(ref, struct fastrpc_invoke_ctx, refcount);
 	cctx = ctx->cctx;
 
-	for (i = 0; i < ctx->nscalars; i++)
-		fastrpc_map_put(ctx->maps[i]);
+	for (i = 0; i < ctx->nscalars; i++) {
+		if (ctx->maps[i])
+			fastrpc_map_put(ctx->maps[i]);
+	}
 
 	if (ctx->buf)
 		fastrpc_buf_free(ctx->buf);
@@ -1103,12 +1107,8 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 	fl->init_mem = NULL;
 	fastrpc_buf_free(imem);
 err_alloc:
-	if (map) {
-		spin_lock(&fl->lock);
-		list_del(&map->node);
-		spin_unlock(&fl->lock);
+	if (map)
 		fastrpc_map_put(map);
-	}
 err:
 	kfree(args);
 
@@ -1185,10 +1185,9 @@ static int fastrpc_device_release(struct inode *inode, struct file *file)
 		fastrpc_context_put(ctx);
 	}
 
-	list_for_each_entry_safe(map, m, &fl->maps, node) {
-		list_del(&map->node);
+	list_for_each_entry_safe(map, m, &fl->maps, node)
 		fastrpc_map_put(map);
-	}
+	WARN_ON(!list_empty(&fl->maps));
 
 	list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
 		list_del(&buf->node);
-- 
2.26.1

[PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call

[Jonathan Marek]

fastrpc_internal_invoke() will call fastrpc_map_create, so there is no
point in having it called here. This does change the behavior somewhat as
fastrpc_internal_invoke() will release the map afterwards, but that's what
we want to happen in this case.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 drivers/misc/fastrpc.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 170352b43ab6..ccad9f5f5e2f 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1013,7 +1013,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 	struct fastrpc_init_create init;
 	struct fastrpc_invoke_args *args;
 	struct fastrpc_phy_page pages[1];
-	struct fastrpc_map *map = NULL;
 	struct fastrpc_buf *imem = NULL;
 	int memlen;
 	int err;
@@ -1049,18 +1048,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 	inbuf.siglen = init.siglen;
 	fl->pd = USER_PD;
 
-	if (init.filelen && init.filefd) {
-		err = fastrpc_map_create(fl, init.filefd, init.filelen, &map);
-		if (err)
-			goto err;
-	}
-
 	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
 		       1024 * 1024);
 	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
 				&imem);
 	if (err)
-		goto err_alloc;
+		goto err;
 
 	fl->init_mem = imem;
 	args[0].ptr = (u64)(uintptr_t)&inbuf;
@@ -1106,9 +1099,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
 err_invoke:
 	fl->init_mem = NULL;
 	fastrpc_buf_free(imem);
-err_alloc:
-	if (map)
-		fastrpc_map_put(map);
 err:
 	kfree(args);
 
-- 
2.26.1

Re: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call

[Srinivas Kandagatla]

On 18/02/2021 03:20, Jonathan Marek wrote:
> fastrpc_internal_invoke() will call fastrpc_map_create, so there is no
> point in having it called here. This does change the behavior somewhat as
> fastrpc_internal_invoke() will release the map afterwards, but that's what
> we want to happen in this case.

This will crash the DSP as you will be freeing the init process memory 
while it is actively using it!

The shell/init process is created as part of user process and it should 
be valid until the user process is valid! We can not free it when the 
invoke is finished/acked as we normally do for other invoke context!

In some firmwares the shell process is statically built into the DSP 
firmware which might work! But other normal cases are totally broken by 
this patch!

--srini

> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>   drivers/misc/fastrpc.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 170352b43ab6..ccad9f5f5e2f 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -1013,7 +1013,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   	struct fastrpc_init_create init;
>   	struct fastrpc_invoke_args *args;
>   	struct fastrpc_phy_page pages[1];
> -	struct fastrpc_map *map = NULL;
>   	struct fastrpc_buf *imem = NULL;
>   	int memlen;
>   	int err;
> @@ -1049,18 +1048,12 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   	inbuf.siglen = init.siglen;
>   	fl->pd = USER_PD;
>   
> -	if (init.filelen && init.filefd) {
> -		err = fastrpc_map_create(fl, init.filefd, init.filelen, &map);
> -		if (err)
> -			goto err;
> -	}
> - >   	memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
>   		       1024 * 1024);
>   	err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
>   				&imem);
>   	if (err)
> -		goto err_alloc;
> +		goto err;
>   
>   	fl->init_mem = imem;
>   	args[0].ptr = (u64)(uintptr_t)&inbuf;
> @@ -1106,9 +1099,6 @@ static int fastrpc_init_create_process(struct fastrpc_user *fl,
>   err_invoke:
>   	fl->init_mem = NULL;
>   	fastrpc_buf_free(imem);
> -err_alloc:
> -	if (map)
> -		fastrpc_map_put(map);
>   err:
>   	kfree(args);
>   
> 

Re: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call

[Jonathan Marek]

On 2/22/21 7:37 AM, Srinivas Kandagatla wrote:
> 
> 
> On 18/02/2021 03:20, Jonathan Marek wrote:
>> fastrpc_internal_invoke() will call fastrpc_map_create, so there is no
>> point in having it called here. This does change the behavior somewhat as
>> fastrpc_internal_invoke() will release the map afterwards, but that's 
>> what
>> we want to happen in this case.
> 
> This will crash the DSP as you will be freeing the init process memory 
> while it is actively using it!
> 
> The shell/init process is created as part of user process and it should 
> be valid until the user process is valid! We can not free it when the 
> invoke is finished/acked as we normally do for other invoke context!
> 
> In some firmwares the shell process is statically built into the DSP 
> firmware which might work! But other normal cases are totally broken by 
> this patch!
> 
> --srini
> 

I am not using the static guest process, I am using the 
FASTRPC_IOCTL_INIT_CREATE to load a fastrpc shell process. It doesn't crash.

AFAIK the DSP does not need the process memory after the process 
creation - this would allow userspace to modify the executable after the 
DSP verifies the hash/signature. So the DSP absolutely needs to make a 
copy of it before verifying it (otherwise this would be a pretty serious 
and obvious security flaw in qcom's fastrpc system. but I wouldn't be 
surprised!).

>>
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   drivers/misc/fastrpc.c | 12 +-----------
>>   1 file changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>> index 170352b43ab6..ccad9f5f5e2f 100644
>> --- a/drivers/misc/fastrpc.c
>> +++ b/drivers/misc/fastrpc.c
>> @@ -1013,7 +1013,6 @@ static int fastrpc_init_create_process(struct 
>> fastrpc_user *fl,
>>       struct fastrpc_init_create init;
>>       struct fastrpc_invoke_args *args;
>>       struct fastrpc_phy_page pages[1];
>> -    struct fastrpc_map *map = NULL;
>>       struct fastrpc_buf *imem = NULL;
>>       int memlen;
>>       int err;
>> @@ -1049,18 +1048,12 @@ static int fastrpc_init_create_process(struct 
>> fastrpc_user *fl,
>>       inbuf.siglen = init.siglen;
>>       fl->pd = USER_PD;
>> -    if (init.filelen && init.filefd) {
>> -        err = fastrpc_map_create(fl, init.filefd, init.filelen, &map);
>> -        if (err)
>> -            goto err;
>> -    }
>> - >       memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
>>                  1024 * 1024);
>>       err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
>>                   &imem);
>>       if (err)
>> -        goto err_alloc;
>> +        goto err;
>>       fl->init_mem = imem;
>>       args[0].ptr = (u64)(uintptr_t)&inbuf;
>> @@ -1106,9 +1099,6 @@ static int fastrpc_init_create_process(struct 
>> fastrpc_user *fl,
>>   err_invoke:
>>       fl->init_mem = NULL;
>>       fastrpc_buf_free(imem);
>> -err_alloc:
>> -    if (map)
>> -        fastrpc_map_put(map);
>>   err:
>>       kfree(args);
>>

Re: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call

[Srinivas Kandagatla]

On 22/02/2021 13:53, Jonathan Marek wrote:
> On 2/22/21 7:37 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 18/02/2021 03:20, Jonathan Marek wrote:
>>> fastrpc_internal_invoke() will call fastrpc_map_create, so there is no
>>> point in having it called here. This does change the behavior 
>>> somewhat as
>>> fastrpc_internal_invoke() will release the map afterwards, but that's 
>>> what
>>> we want to happen in this case.
>>
>> This will crash the DSP as you will be freeing the init process memory 
>> while it is actively using it!
>>
>> The shell/init process is created as part of user process and it 
>> should be valid until the user process is valid! We can not free it 
>> when the invoke is finished/acked as we normally do for other invoke 
>> context!
>>
>> In some firmwares the shell process is statically built into the DSP 
>> firmware which might work! But other normal cases are totally broken 
>> by this patch!
>>
>> --srini
>>
> 
> I am not using the static guest process, I am using the 
> FASTRPC_IOCTL_INIT_CREATE to load a fastrpc shell process. It doesn't 
> crash.
> 
> AFAIK the DSP does not need the process memory after the process 
> creation - this would allow userspace to modify the executable after the 

This process memory includes memory to load shell and the heap(complete 
process memory) so it should be kept till the process dies!

--srini


> DSP verifies the hash/signature. So the DSP absolutely needs to make a 
> copy of it before verifying it (otherwise this would be a pretty serious 
> and obvious security flaw in qcom's fastrpc system. but I wouldn't be 
> surprised!).




> 
>>>
>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>> ---
>>>   drivers/misc/fastrpc.c | 12 +-----------
>>>   1 file changed, 1 insertion(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>>> index 170352b43ab6..ccad9f5f5e2f 100644
>>> --- a/drivers/misc/fastrpc.c
>>> +++ b/drivers/misc/fastrpc.c
>>> @@ -1013,7 +1013,6 @@ static int fastrpc_init_create_process(struct 
>>> fastrpc_user *fl,
>>>       struct fastrpc_init_create init;
>>>       struct fastrpc_invoke_args *args;
>>>       struct fastrpc_phy_page pages[1];
>>> -    struct fastrpc_map *map = NULL;
>>>       struct fastrpc_buf *imem = NULL;
>>>       int memlen;
>>>       int err;
>>> @@ -1049,18 +1048,12 @@ static int fastrpc_init_create_process(struct 
>>> fastrpc_user *fl,
>>>       inbuf.siglen = init.siglen;
>>>       fl->pd = USER_PD;
>>> -    if (init.filelen && init.filefd) {
>>> -        err = fastrpc_map_create(fl, init.filefd, init.filelen, &map);
>>> -        if (err)
>>> -            goto err;
>>> -    }
>>> - >       memlen = ALIGN(max(INIT_FILELEN_MAX, (int)init.filelen * 4),
>>>                  1024 * 1024);
>>>       err = fastrpc_buf_alloc(fl, fl->sctx->dev, memlen,
>>>                   &imem);
>>>       if (err)
>>> -        goto err_alloc;
>>> +        goto err;
>>>       fl->init_mem = imem;
>>>       args[0].ptr = (u64)(uintptr_t)&inbuf;
>>> @@ -1106,9 +1099,6 @@ static int fastrpc_init_create_process(struct 
>>> fastrpc_user *fl,
>>>   err_invoke:
>>>       fl->init_mem = NULL;
>>>       fastrpc_buf_free(imem);
>>> -err_alloc:
>>> -    if (map)
>>> -        fastrpc_map_put(map);
>>>   err:
>>>       kfree(args);
>>>