Re: [PATCH 4/4] media: tegra-cec: fix possible object reference leak

Re: [PATCH 4/4] media: tegra-cec: fix possible object reference leak

[Hans Verkuil (hansverk)]

On 09/02/2019 03:55, Wen Yang wrote:
> The call to of_parse_phandle() returns a node pointer with refcount
> incremented thus it must be explicitly decremented here after the last
> usage.
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we also should release that reference.
> This patch fixes those two issues.
> 
> Fixes: 9d2d60687c9a ("media: tegra-cec: add Tegra HDMI CEC driver")
> Signed-off-by: Wen Yang <yellowriver2010@hotmail.com>
> ---
>  drivers/media/platform/tegra-cec/tegra_cec.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/tegra-cec/tegra_cec.c b/drivers/media/platform/tegra-cec/tegra_cec.c
> index aba488c..b6c28c8 100644
> --- a/drivers/media/platform/tegra-cec/tegra_cec.c
> +++ b/drivers/media/platform/tegra-cec/tegra_cec.c
> @@ -340,19 +340,24 @@ static int tegra_cec_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  	hdmi_dev = of_find_device_by_node(np);
> -	if (hdmi_dev == NULL)
> +	if (hdmi_dev == NULL) {
> +		of_node_put(np);
>  		return -EPROBE_DEFER;
> +	}
> +	of_node_put(np);

You can move this line to just after the 'hdmi_dev = of_find_device_by_node(np);'
line.

>  
>  	cec = devm_kzalloc(&pdev->dev, sizeof(struct tegra_cec), GFP_KERNEL);
> -
> -	if (!cec)
> +	if (!cec) {
> +		put_device(&hdmi_dev->dev);

You don't need to do this. In fact, the put_device can be done before the
cec = devm_kzalloc line.

There is no need for this driver to keep a reference to the hdmi device, the
device pointer is only used as a key in the notifier list. This cec driver
will never access the hdmi device.

There are several other CEC drivers that have this same mistake and that
need to put the hdmi_dev device.

Regards,

	Hans

>  		return -ENOMEM;
> +	}
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
>  	if (!res) {
>  		dev_err(&pdev->dev,
>  			"Unable to allocate resources for device\n");
> +		put_device(&hdmi_dev->dev);
>  		return -EBUSY;
>  	}
>  
> @@ -360,19 +365,23 @@ static int tegra_cec_probe(struct platform_device *pdev)
>  		pdev->name)) {
>  		dev_err(&pdev->dev,
>  			"Unable to request mem region for device\n");
> +		put_device(&hdmi_dev->dev);
>  		return -EBUSY;
>  	}
>  
>  	cec->tegra_cec_irq = platform_get_irq(pdev, 0);
>  
> -	if (cec->tegra_cec_irq <= 0)
> +	if (cec->tegra_cec_irq <= 0) {
> +		put_device(&hdmi_dev->dev);
>  		return -EBUSY;
> +	}
>  
>  	cec->cec_base = devm_ioremap_nocache(&pdev->dev, res->start,
>  					     resource_size(res));
>  
>  	if (!cec->cec_base) {
>  		dev_err(&pdev->dev, "Unable to grab IOs for device\n");
> +		put_device(&hdmi_dev->dev);
>  		return -EBUSY;
>  	}
>  
> @@ -380,6 +389,7 @@ static int tegra_cec_probe(struct platform_device *pdev)
>  
>  	if (IS_ERR_OR_NULL(cec->clk)) {
>  		dev_err(&pdev->dev, "Can't get clock for CEC\n");
> +		put_device(&hdmi_dev->dev);
>  		return -ENOENT;
>  	}
>  
> @@ -397,12 +407,14 @@ static int tegra_cec_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(&pdev->dev,
>  			"Unable to request interrupt for device\n");
> +		put_device(&hdmi_dev->dev);
>  		goto clk_error;
>  	}
>  
>  	cec->notifier = cec_notifier_get(&hdmi_dev->dev);
>  	if (!cec->notifier) {
>  		ret = -ENOMEM;
> +		put_device(&hdmi_dev->dev);
>  		goto clk_error;
>  	}
>  
>