Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V1] nvmem: core: fix memory abort in cleanup path
To:     Thierry Reding <treding@nvidia.com>
CC:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh@kernel.org>, <linux-kernel@vger.kernel.org>,
        Jonathan Hunter <jonathanh@nvidia.com>
References: <1577592162-14817-1-git-send-email-bbiswas@nvidia.com>
 <20200102124445.GB1924669@ulmo>
 <7abb79c6-b497-98b3-45ff-44d751f1c781@nvidia.com>
 <20200103071152.GA1933715@ulmo>
From:   Bitan Biswas <bbiswas@nvidia.com>
Message-ID: <e56ff5c6-04b1-3b2b-8ff4-9e416e143dee@nvidia.com>
Date:   Fri, 3 Jan 2020 04:50:10 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20200103071152.GA1933715@ulmo>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Thierry,

On 1/2/20 11:11 PM, Thierry Reding wrote:
> On Thu, Jan 02, 2020 at 10:51:24AM -0800, Bitan Biswas wrote:
>>
>> Hi Thierry,
>>
>> On 1/2/20 4:44 AM, Thierry Reding wrote:
>>> On Sat, Dec 28, 2019 at 08:02:42PM -0800, Bitan Biswas wrote:
>>>> nvmem_cell_info_to_nvmem_cell implementation has static
>>>> allocation of name. nvmem_add_cells_from_of() call may
>>>> return error and kfree name results in memory abort. Use
>>>> kasprintf() instead of assigning pointer and prevent kfree crash.
>>>>
>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>> index 9f1ee9c..0fc66e1 100644
>>>> --- a/drivers/nvmem/core.c
>>>> +++ b/drivers/nvmem/core.c
>>>> @@ -110,7 +110,7 @@ static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
>>>>    	cell->nvmem = nvmem;
>>>>    	cell->offset = info->offset;
>>>>    	cell->bytes = info->bytes;
>>>> -	cell->name = info->name;
>>>> +	cell->name = kasprintf(GFP_KERNEL, "%s", info->name);
>>
>>>
>>> kstrdup() seems more appropriate here.
>> Thanks. I shall update the patch as suggested.
>>
>>>
>>> A slightly more efficient way to do this would be to use a combination
>>> of kstrdup_const() and kfree_const(), which would allow read-only
>>> strings to be replicated by simple assignment rather than duplication.
>>> Note that in that case you'd need to carefully replace all kfree() calls
>>> on cell->name by a kfree_const() to ensure they do the right thing.
>> kfree(cell->name) is also called for allocations in function
>> nvmem_add_cells_from_of() through below call
>> kasprintf(GFP_KERNEL, "%pOFn", child);
>>
>> My understanding is kfree_const may not work for above allocation.
> 
> kfree_const() checks the location that the pointer passed to it points
> to. If it points to the kernel's .rodata section, it returns and only
> calls kfree() otherwise. Similarily, kstrdup_const() returns its
> argument if it points to the .rodata section and duplicates the string
> otherwise. On the other hand, pointers returned by kasprintf() will
> never point to the .rodata section, so kfree_const() will result in
> kfree() getting called.
> 
> That said, the savings here are fairly minimal, so I don't feel very
> strongly about this. Feel free to go with the kstrdup() variant.
Thanks for the explanation. I would test the implementation with the 
_const functions you suggested and send updated patch.

-regards,
  Bitan