linux-kernel - Re: [PATCH 3/3] fastrpc: remove redundant 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]

Date:   Mon, 22 Feb 2021 08:53:29 -0500
From:   Jonathan Marek <jonathan@...ek.ca>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-arm-msm@...r.kernel.org
Cc:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call

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@...ek.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);
>>