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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a2f22968-3c3c-893e-873e-c7d85b32d212@axentia.se>
Date:   Tue, 4 Dec 2018 14:50:44 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Nicholas Mc Guire <der.herr@...r.at>
CC:     Nicholas Mc Guire <hofrat@...dl.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "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()

On 2018-12-04 15:36, Nicholas Mc Guire wrote:
> On Tue, Dec 04, 2018 at 02:11:33PM +0000, Peter Rosin wrote:
>> 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
>>
> 
> yup - which is where the question in the initial pach came
> from I felt that this should better be a devm_kasprint() - I
> did not understand why it was kstrdup here in the first
> place - technically it is not wrong though as the source is not RO
> so it will allocate and copy the original string and thus
> effectively it behaves like devm_kasprintf

devm_kasprintf would be as bad as devm_kstrdup here (if I read the code
right). Both allocates memory that is freed both by the devm_... machinery
and by the consumer. Maybe I should have said "props consumer" instead of
just "consumer" to make it clear that I'm talking about the dynamic of
code as consumer.

>  
>> 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.
> 
> Why would it be freed twice ? the pointer returned is a seperately allocated objects ?
> that needs to be indepdently freed (this is not the devm_kstrdup_const() case)

The props[i] struct is sent off to the "consumer" with the call to
of_changeset_update_property. After that (if I read it right) the
changeset owns the prop and will free it, and its content, as it is
cleaned up. It is therefore wrong for this driver to also clean up
that memory (by using devm_ prefixed functions for these two
entities).

>>
>> So, there is no case, AFAICT, where it is sane to use devm_kstrdup.
> 
> I think the only technical difference between devm_kstrdup and devm_kasprintf
> is effectively a memcpy vs vsnprintf - so here I think devm_kasprintf would
> also be the more suitable call to use.

That difference is not very important, but strdup should always be faster
than asprintf, so I see no point in not using strdup.

> thx!
> hofrat
> 
>>
>> 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);
>>>
>>