From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rui Miguel Silva Date: Thu, 29 Aug 2019 14:53:20 +0000 Subject: Re: [PATCH] staging: greybus: light: fix a couple double frees Message-Id: List-Id: References: <20190829122839.GA20116@mwanda> In-Reply-To: <20190829122839.GA20116@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Johan Hovold , Alex Elder , Greg Kroah-Hartman , greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Hi Dan, On Thu 29 Aug 2019 at 13:28, Dan Carpenter wrote: > The problem is in gb_lights_request_handler(). If we get a request to > change the config then we release the light with gb_lights_light_release() > and re-allocated it. However, if the allocation fails part way through > then we call gb_lights_light_release() again. This can lead to a couple > different double frees where we haven't cleared out the original values: > > gb_lights_light_v4l2_unregister(light); > ... > kfree(light->channels); > kfree(light->name); > > I also made a small change to how we set "light->channels_count = 0;". > The original code handled this part fine and did not cause a use after > free but it was sort of complicated to read. > > Fixes: 2870b52bae4c ("greybus: lights: add lights implementation") > Signed-off-by: Dan Carpenter > Thanks so much for this, I was looking for some time at this and was half way to a much less elegant fix then yours. Acked-by: Rui Miguel Silva Cheers, Rui > --- > drivers/staging/greybus/light.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c > index 010ae1e9c7fb..40680eaf3974 100644 > --- a/drivers/staging/greybus/light.c > +++ b/drivers/staging/greybus/light.c > @@ -1098,21 +1098,21 @@ static void gb_lights_channel_release(struct gb_channel *channel) > static void gb_lights_light_release(struct gb_light *light) > { > int i; > - int count; > > light->ready = false; > > - count = light->channels_count; > - > if (light->has_flash) > gb_lights_light_v4l2_unregister(light); > + light->has_flash = false; > > - for (i = 0; i < count; i++) { > + for (i = 0; i < light->channels_count; i++) > gb_lights_channel_release(&light->channels[i]); > - light->channels_count--; > - } > + light->channels_count = 0; > + > kfree(light->channels); > + light->channels = NULL; > kfree(light->name); > + light->name = NULL; > } > > static void gb_lights_release(struct gb_lights *glights)