| Message ID | 20240604-dell-pc-double-free-v1-1-6d81255b2a44@weissschuh.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: dell-pc: avoid double free and invalid unregistration | expand |
On Tue, Jun 4 2024 at 11:41:24 PM +02:00:00, Thomas Weißschuh <linux@weissschuh.net> wrote: > If platform_profile_register() fails it does kfree(thermal_handler) > and > leaves the pointer value around. > Any call to thermal_cleanup() will try to kfree(thermal_handler) > again. > This will happen right away in dell_init(). > In addition, platform_profile_remove() will be called although no > profile is registered. > > NULL out the thermal_handler, so thermal_cleanup() avoids the double > free. > > Fixes: 996ad4129810 ("platform/x86: dell-pc: Implement > platform_profile") > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> > --- > Currently the call to thermal_cleanup() in dell_init() is completely > unnecessary. But I guess more functionality will be handled by the > driver and then this structure makes sense. > > This is untested. > --- > drivers/platform/x86/dell/dell-pc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/dell/dell-pc.c > b/drivers/platform/x86/dell/dell-pc.c > index dfe09c463d03..972385ca1990 100644 > --- a/drivers/platform/x86/dell/dell-pc.c > +++ b/drivers/platform/x86/dell/dell-pc.c > @@ -261,8 +261,10 @@ static int thermal_init(void) > > /* Clean up if failed */ > ret = platform_profile_register(thermal_handler); > - if (ret) > + if (ret) { > kfree(thermal_handler); > + thermal_handler = NULL; > + } > > return ret; > } > > --- > base-commit: 0da7a954480cc99978e3570c991e3779e56fc736 > change-id: 20240604-dell-pc-double-free-e8cf2aa9b2fb > > Best regards, > -- > Thomas Weißschuh <linux@weissschuh.net> Thank you for this patch. I was able to apply your patch and compile successfully. I have not had a chance to try it on my hardware. I agree with the change, and it looks good to me. Thanks, Reviewed-by: Lyndon Sanche <lsanche@lyndeno.ca>
On Tue, 04 Jun 2024 23:41:24 +0200, Thomas Weißschuh wrote: > If platform_profile_register() fails it does kfree(thermal_handler) and > leaves the pointer value around. > Any call to thermal_cleanup() will try to kfree(thermal_handler) again. > This will happen right away in dell_init(). > In addition, platform_profile_remove() will be called although no > profile is registered. > > [...] Thank you for your contribution, it has been applied to my local review-ilpo branch. Note it will show up in the public platform-drivers-x86/review-ilpo branch only once I've pushed my local branch there, which might take a while. The list of commits applied: [1/1] platform/x86: dell-pc: avoid double free and invalid unregistration commit: dd637f5cd5f334d2d014872544470031415cec3b -- i.
diff --git a/drivers/platform/x86/dell/dell-pc.c b/drivers/platform/x86/dell/dell-pc.c index dfe09c463d03..972385ca1990 100644 --- a/drivers/platform/x86/dell/dell-pc.c +++ b/drivers/platform/x86/dell/dell-pc.c @@ -261,8 +261,10 @@ static int thermal_init(void) /* Clean up if failed */ ret = platform_profile_register(thermal_handler); - if (ret) + if (ret) { kfree(thermal_handler); + thermal_handler = NULL; + } return ret; }
If platform_profile_register() fails it does kfree(thermal_handler) and leaves the pointer value around. Any call to thermal_cleanup() will try to kfree(thermal_handler) again. This will happen right away in dell_init(). In addition, platform_profile_remove() will be called although no profile is registered. NULL out the thermal_handler, so thermal_cleanup() avoids the double free. Fixes: 996ad4129810 ("platform/x86: dell-pc: Implement platform_profile") Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- Currently the call to thermal_cleanup() in dell_init() is completely unnecessary. But I guess more functionality will be handled by the driver and then this structure makes sense. This is untested. --- drivers/platform/x86/dell/dell-pc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) --- base-commit: 0da7a954480cc99978e3570c991e3779e56fc736 change-id: 20240604-dell-pc-double-free-e8cf2aa9b2fb Best regards,