staging: greybus: light: fix a couple double frees
Subsystems (4)
PATCH 1/1 staging: greybus: light: fix a couple double frees
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.
--- drivers/staging/greybus/light.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/staging/greybus/light.c +6 -6
@@ -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)
Signature · 1 line
2.20.1
Hi Dan,
Quoted 11 lines · Dan Carpenter
The problem is in
gb_lights_request_handler(). If we get a request to change the config then we release the light withgb_lights_light_release()and re-allocated it. However, if the allocation fails part way through then we callgb_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);
Quoted 7 lines
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 <[email protected]>
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.
Cheers,
Rui
Quoted 36 lines
--- drivers/staging/greybus/light.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/staging/greybus/light.c +6
-6@@ -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)