lkml.org 
[lkml]   [2021]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call
From
Date
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);
>>

\
 
 \ /
  Last update: 2021-02-22 15:40    [W:0.046 / U:24.780 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site