linux-kernel - Re: [PATCH] i2c: mux: demux-pinctrl: handle failure case of devm

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: <37b8709c-0b58-7bc9-22c5-e01953593d06@axentia.se>
Date:   Tue, 4 Dec 2018 14:11:33 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Nicholas Mc Guire <hofrat@...dl.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>
CC:     "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] i2c: mux: demux-pinctrl: handle failure case of
 devm_kstrdup()

Ho hmm. Had another look at this patch, or rather, the context of the
patch so not really related, but...

On 2018-12-01 11:01, Nicholas Mc Guire wrote:
> devm_kstrdup() may return NULL if internal allocation failed.
> Thus using  name, value  is unsafe without being checked. As
> i2c_demux_pinctrl_probe() can return -ENOMEM in other cases
> a dev_err() message is included to make the failure location
> clear.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> Fixes: e35478eac030 ("i2c: mux: demux-pinctrl: run properly with multiple instances")
> ---
> 
> Problem located with experimental coccinelle script
> 
> Q: The use of devm_kstrdup() seems a bit odd while technically not wrong,
>    personally I think devm_kasprintf() would be more suitable here.
> 
> Patch was compile tested with: multi_v7_defconfig 
> (implies I2C_DEMUX_PINCTRL=y)
> 
> Patch is against 4.20-rc4 (localversion-next is next-20181130)
> 
>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> index 035032e..c466999 100644
> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
> @@ -244,6 +244,12 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
>  
>  		props[i].name = devm_kstrdup(&pdev->dev, "status", GFP_KERNEL);
>  		props[i].value = devm_kstrdup(&pdev->dev, "ok", GFP_KERNEL);

It seemed very dubious to use devm_kstrdup here, since

1. if the consumer is not freeing the strings it would be sufficient with just
  		props[i].name = "status";
  		props[i].value = "ok";

2. if the consumer is freeing the strings, it is very bad to free them twice
   which is what happens with the devm_ prefix.

So, there is no case, AFAICT, where it is sane to use devm_kstrdup.

Therefore I had a look at the code, and to me it seems as if the consumer
very much frees the strings, which means that we are in case 2, and that
the above should be ordinary kstrdup calls.

Am I missing something?

Cheers,
Peter

> +		if (!props[i].name || !props[i].value) {
> +			dev_err(&pdev->dev,
> +				"chan %d name, value allocation failed\n", i);
> +			err = -ENOMEM;
> +			goto err_rollback;
> +		}
>  		props[i].length = 3;
>  
>  		of_changeset_init(&priv->chan[i].chgset);
>