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]

Message-ID: <58e361e0-441e-fd71-362a-398dcb84f888@linaro.org>
Date:   Mon, 22 Feb 2021 12:37:04 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Jonathan Marek <jonathan@...ek.ca>, 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 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@...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);
>   
>