Messages in this thread | | | Subject | Re: [PATCH 3/3] fastrpc: remove redundant fastrpc_map_create() call | From | Jonathan Marek <> | Date | Mon, 22 Feb 2021 08:53:29 -0500 |
| |
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); >>
| |