Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] nvmem: core: fix possibly memleak when use
 nvmem_cell_info_to_nvmem_cell()
To:     Vadym Kochan <vadym.kochan@plvision.eu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
References: <20200923135343.16565-1-vadym.kochan@plvision.eu>
 <7a072204-a4b8-e5ef-9ce2-4db176f70366@linaro.org>
 <20200923141334.GA16798@plvision.eu>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <1f65dc6a-50f3-d4e5-f1ce-7a68fddde287@linaro.org>
Date:   Wed, 23 Sep 2020 15:47:14 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20200923141334.GA16798@plvision.eu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 23/09/2020 15:13, Vadym Kochan wrote:
> On Wed, Sep 23, 2020 at 03:10:36PM +0100, Srinivas Kandagatla wrote:
>>
>>
>> On 23/09/2020 14:53, Vadym Kochan wrote:
>>> Fix missing 'kfree_const(cell->name)' when call to
>>> nvmem_cell_info_to_nvmem_cell() in several places:
>>>
>>>        * after nvmem_cell_info_to_nvmem_cell() failed during
>>>          nvmem_add_cells()
>>>
>>>        * during nvmem_device_cell_{read,write}. This is fixed by simply
>>>          re-using info->name instead of duplicating it:
>>>
>>>              cell->name = info->name
>>>
>>> Because cell->name is not used except for error message printing in case
>>> of un-aligned access, the new __nvmem_cell_info_to_nvmem_cell() helper
>>> was introduced.
>>>
>>> Fixes: e2a5402ec7c6 ("nvmem: Add nvmem_device based consumer apis.")
>>> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
>>> ---
>>> v2:
>>>       * remove not needed 'kfree_const(cell->name)' after nvmem_cell_info_to_nvmem_cell()
>>>         failed.
>>>
>>>    drivers/nvmem/core.c | 35 ++++++++++++++++++++++++++---------
>>>    1 file changed, 26 insertions(+), 9 deletions(-)
>>
>>
>>
>> Really :-)
>>
> But what about nvmem_device_cell_{read,write} case ?
> In my understanding the cell is allocated on the stack but kstrdup() is
You are right!

That is the second issue where the caller outside would fail after 
successful call to nvmem_cell_info_to_nvmem_cell() .

Probably we cam free it in failure cases!
something like:

------------------------>cut<---------------------------
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6cd3edb2eaf6..fb1e756adcee 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -383,6 +383,7 @@ static int nvmem_cell_info_to_nvmem_cell(struct 
nvmem_device *nvmem,
                 dev_err(&nvmem->dev,
                         "cell %s unaligned to nvmem stride %d\n",
                         cell->name, nvmem->stride);
+               kfree_const(cell->name);
                 return -EINVAL;
         }

@@ -1465,8 +1466,10 @@ ssize_t nvmem_device_cell_read(struct 
nvmem_device *nvmem,
                 return rc;

         rc = __nvmem_cell_read(nvmem, &cell, buf, &len);
-       if (rc)
+       if (rc) {
+               kfree_const(cell->name);
                 return rc;
+       }

         return len;
  }
@@ -1494,7 +1497,11 @@ int nvmem_device_cell_write(struct nvmem_device 
*nvmem,
         if (rc)
                 return rc;

-       return nvmem_cell_write(&cell, buf, cell.bytes);
+       rc = nvmem_cell_write(&cell, buf, cell.bytes);
+       if (rc)
+               kfree_const(cell->name);
+
+       return rc;
  }
  EXPORT_SYMBOL_GPL(nvmem_device_cell_write);
  ------------------------>cut<---------------------------

--srini

> not freed in the end, or I missed something ?
> 
>>
>> Below change should just fix this the reported issue!
>> ------------------------>cut<---------------------------
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 6cd3edb2eaf6..9fb9112fe75d 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -383,6 +383,7 @@ static int nvmem_cell_info_to_nvmem_cell(struct
>> nvmem_device *nvmem,
>>                  dev_err(&nvmem->dev,
>>                          "cell %s unaligned to nvmem stride %d\n",
>>                          cell->name, nvmem->stride);
>> +               kfree_const(cell->name);
>>                  return -EINVAL;
>>          }
>>
>> ------------------------>cut<---------------------------
>>
>> I don't see a point in the way your patch try to fix this!!
>>
>>
>> --srini
>>
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index 6cd3edb2eaf6..e6d1bc414faf 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -361,16 +361,15 @@ static void nvmem_cell_add(struct nvmem_cell *cell)
>>>    	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
>>>    }
>>> -static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
>>> -				   const struct nvmem_cell_info *info,
>>> -				   struct nvmem_cell *cell)
>>> +static int
>>> +__nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
>>> +				const struct nvmem_cell_info *info,
>>> +				struct nvmem_cell *cell)
>>>    {
>>>    	cell->nvmem = nvmem;
>>>    	cell->offset = info->offset;
>>>    	cell->bytes = info->bytes;
>>> -	cell->name = kstrdup_const(info->name, GFP_KERNEL);
>>> -	if (!cell->name)
>>> -		return -ENOMEM;
>>> +	cell->name = info->name;
>>>    	cell->bit_offset = info->bit_offset;
>>>    	cell->nbits = info->nbits;
>>> @@ -382,13 +381,31 @@ static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
>>>    	if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
>>>    		dev_err(&nvmem->dev,
>>>    			"cell %s unaligned to nvmem stride %d\n",
>>> -			cell->name, nvmem->stride);
>>> +			cell->name ?: "<unknown>", nvmem->stride);
>>>    		return -EINVAL;
>>>    	}
>>>    	return 0;
>>>    }
>>> +static int
>>> +nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
>>> +			      const struct nvmem_cell_info *info,
>>> +			      struct nvmem_cell *cell)
>>> +{
>>> +	int err;
>>> +
>>> +	err = __nvmem_cell_info_to_nvmem_cell(nvmem, info, cell);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	cell->name = kstrdup_const(info->name, GFP_KERNEL);
>>> +	if (!cell->name)
>>> +		return -ENOMEM;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    /**
>>>     * nvmem_add_cells() - Add cell information to an nvmem device
>>>     *
>>> @@ -1460,7 +1477,7 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
>>>    	if (!nvmem)
>>>    		return -EINVAL;
>>> -	rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
>>> +	rc = __nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
>>>    	if (rc)
>>>    		return rc;
>>> @@ -1490,7 +1507,7 @@ int nvmem_device_cell_write(struct nvmem_device *nvmem,
>>>    	if (!nvmem)
>>>    		return -EINVAL;
>>> -	rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
>>> +	rc = __nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
>>>    	if (rc)
>>>    		return rc;
>>>