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

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>] [day] [month] [year] [list]

Date:   Mon, 11 Feb 2019 10:53:58 +0000
From:   "Hans Verkuil (hansverk)" <hansverk@...co.com>
To:     Wen Yang <yellowriver2010@...mail.com>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Thierry Reding" <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>
CC:     "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] media: tegra-cec: fix possible object reference leak

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@...mail.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;
>  	}
>  
>