[PATCH] ASoC: qdsp6: fix a use after free bug in open()

[Cezary Rojewski]

On 2021-12-17 4:00 PM, Dan Carpenter wrote:
> This code frees "graph" and then dereferences to save the error code.
> Save the error code first and then use gotos to unwind the allocation.
> 
> Fixes: 59716aa3f976 ("ASoC: qdsp6: Fix an IS_ERR() vs NULL bug")
> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
> ---
>   sound/soc/qcom/qdsp6/q6apm.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/qcom/qdsp6/q6apm.c b/sound/soc/qcom/qdsp6/q6apm.c
> index 3e007d609a9b..f424d7aa389a 100644
> --- a/sound/soc/qcom/qdsp6/q6apm.c
> +++ b/sound/soc/qcom/qdsp6/q6apm.c
> @@ -615,7 +615,7 @@ struct q6apm_graph *q6apm_graph_open(struct device *dev, q6apm_cb cb,
>   	graph = kzalloc(sizeof(*graph), GFP_KERNEL);
>   	if (!graph) {
>   		ret = -ENOMEM;
> -		goto err;
> +		goto put_ar_graph;
>   	}
>   
>   	graph->apm = apm;
> @@ -631,13 +631,15 @@ struct q6apm_graph *q6apm_graph_open(struct device *dev, q6apm_cb cb,
>   
>   	graph->port = gpr_alloc_port(apm->gdev, dev, graph_callback, graph);
>   	if (IS_ERR(graph->port)) {
> -		kfree(graph);
>   		ret = PTR_ERR(graph->port);
> -		goto err;
> +		goto free_graph;
>   	}
>   
>   	return graph;
> -err:
> +
> +free_graph:
> +	kfree(graph);
> +put_ar_graph:

Hello Dan,

The patch looks good! My only suggestion is a readability improvement, 
but I'm unaware of the convention chosen for qcom directory so you may 
choose to ignore it:

Function q6apm_graph_open() has two separate return paths: a happy path 
ending in 'return graph' and an error path which eventually ends with 
'return ERR_PTR(ret)'. Current goto label-naming convention suggests 
it's a happy path nonetheless.

s/free_graph/err_alloc_port/ and s/put_ar_graph/err_alloc_graph/ tells 
reader upfront that they are in the error path.


Regards,
Czarek

>   	kref_put(&ar_graph->refcount, q6apm_put_audioreach_graph);
>   	return ERR_PTR(ret);
>   }
>