From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934121AbcBQKs7 (ORCPT ); Wed, 17 Feb 2016 05:48:59 -0500 Received: from mail-wm0-f50.google.com ([74.125.82.50]:34553 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934008AbcBQKs5 (ORCPT ); Wed, 17 Feb 2016 05:48:57 -0500 Subject: Re: [PATCH] nvmem: core: fix error path in nvmem_add_cells() To: Rasmus Villemoes , Maxime Ripard References: <1454965469-6141-1-git-send-email-linux@rasmusvillemoes.dk> Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org From: Srinivas Kandagatla Message-ID: <56C45016.2010806@linaro.org> Date: Wed, 17 Feb 2016 10:48:54 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1454965469-6141-1-git-send-email-linux@rasmusvillemoes.dk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Rasmus, Thanks for the patch, On 08/02/16 21:04, Rasmus Villemoes wrote: > The current code fails to nvmem_cell_drop(cells[0]) - even worse, if > the loop above fails already at i==0, we'll enter an essentially > infinite loop doing nvmem_cell_drop on cells[-1], cells[-2], ... which > is unlikely to end well. I agree, it would fail in case of zero. > > Also, we're not freeing the temporary backing array cells on the error > path. > > Signed-off-by: Rasmus Villemoes > --- > drivers/nvmem/core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index 6fd4e5a5ef4a..1e65eccfea83 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -288,9 +288,11 @@ static int nvmem_add_cells(struct nvmem_device *nvmem, > > return 0; > err: > - while (--i) > + while (i--) > nvmem_cell_drop(cells[i]); No, this will not work. 3 issues, 1> If we enter this err path from nvmem_cell_info_to_nvmem_cell() failures, you would be accessing already freed cells[i]. 2> accessing un-allocated cells[i]. 3> you would be trying to drop cells which are not in the list. This is what you need here to fix it correctly. while (--i >= 0) > > + kfree(cells); This change looks good. > + > return rval; > } > > --srini