resource leak in firmware/arm_scmi driver

resource leak in firmware/arm_scmi driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 984 bytes --]

Hello,

during some janitorial cleanup I stumbled over a resource leak in
drivers/firmware/arm_scmi/driver.c.

The problem is as follows:

scmi_remove() might return early if info->users is non-zero. The driver
core however ignores the return value of scmi_remove() and removes the
device and frees the devm-allocated resources (e.g. *info).

So letting aside that some resources are never freed after a failed call
of scmi_remove(), the user of the scmi node will probably stumble over
accessing freed memory soon. I wouldn't be surprised if that was
exploitable.

I quickly tried to fix this issue, but didn't understand the driver good
enough. I think a fix would involve adding a get_device() call to
scmi_handle_get() to prevent scmi_remove() being called while a handle
exists.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: resource leak in firmware/arm_scmi driver

[Cristian Marussi]

On Mon, Oct 10, 2022 at 10:45:45AM +0200, Uwe Kleine-König wrote:
> Hello,
> 

Hi Uwe,

thanks for reporting this.

> during some janitorial cleanup I stumbled over a resource leak in
> drivers/firmware/arm_scmi/driver.c.
> 
> The problem is as follows:
> 
> scmi_remove() might return early if info->users is non-zero. The driver
> core however ignores the return value of scmi_remove() and removes the
> device and frees the devm-allocated resources (e.g. *info).
> 
> So letting aside that some resources are never freed after a failed call
> of scmi_remove(), the user of the scmi node will probably stumble over
> accessing freed memory soon. I wouldn't be surprised if that was
> exploitable.
> 

At first, I thought that this was only some kind of mess that needed to
be cleaned up in the remove/exit path but without any real way of being
exposed: this *bad* assumption of mine came from the fact that the SCMI
drivers (like scmi-cpufreq scmi-hwmon) register to use the core SCMI stack
services using an exported symbol (scmi_register_driver) and then, additionally,
each SCMI driver willing to use some the core SCMI protocols issues a
try_module_get (since the protocols could have been modularized)

So that in a module scenario (with scmi-module being the core platform
driver:

root@deb-buster-arm64:~# lsmod 
Module                  Size  Used by
scmi_hwmon             16384  0
scmi_module           131072  2 scmi_hwmon

root@deb-buster-arm64:~# rmmod ./scmi-module.ko 
rmmod: ERROR: Module scmi_module is in use by: scmi_hwmon

BUT, then I realized that all of this refcounting is not going to save me
from an explicitly triggered unbind:

root@deb-buster-arm64:~# echo 'firmware:scmi' > /sys/bus/platform/drivers/arm-scmi/unbind 
[11601.798671] arm-scmi firmware:scmi: remove callback returned a non-zero value. This will be ignored.

This will indeed expose the issue you reported.

> I quickly tried to fix this issue, but didn't understand the driver good
> enough. I think a fix would involve adding a get_device() call to
> scmi_handle_get() to prevent scmi_remove() being called while a handle
> exists.
> 

I tried to address the issue with the get_device() calls as you
suggested, but it has really no effect and in fact looking at the code
on the remove path there is nothing that seems to be taken into
consideration to stop a removal (and this indeed is consistent with the
fact that any error returned on the removal/exit path is effectively ignored).

What instead DOES seem to work, it is to add a proper devlink with
device_link_add() declaring any SCMI driver device (like from cpufreq)
as a consumer of the core stack platform device: this will NOT stop the
removal, BUT it does trigger an implicit unbind at first of all the SCMI
drivers currently loaded when an unbind is requested for the core SCMI
platform device: as a consequence the removal can now be triggered
safely by an unbind even if some SCMI driver users are present (they
will be unbound too)

Now, for this cleanup to fully work, some (long due) rework of the SCMI
core devices handling and removal is needed (and it has partially
already started in other unrelated series), so my plan would be to, at
first, immediately add a '.suppress_bind_attrs = true' to the core driver
to mitigate this (which is also easily portable to stable) and then
rework properly the core SCMI removeal routines to safely re-enable
a proper unbind. (or not re-enabling it if deemed not worth BUT anyway
reworking the core SCMI remove/exit path)

I'll check my plan with Sudeep, any thoughts from you about this ?
Am I missing something more ?

Thanks a lot.

Regards,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: resource leak in firmware/arm_scmi driver

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 4349 bytes --]

Hello Cristian,

adding Rafael and Greg to Cc:; maybe they can add some nice alternative.

On Thu, Oct 13, 2022 at 12:48:03PM +0100, Cristian Marussi wrote:
> On Mon, Oct 10, 2022 at 10:45:45AM +0200, Uwe Kleine-König wrote:
> > during some janitorial cleanup I stumbled over a resource leak in
> > drivers/firmware/arm_scmi/driver.c.
> > 
> > The problem is as follows:
> > 
> > scmi_remove() might return early if info->users is non-zero. The driver
> > core however ignores the return value of scmi_remove() and removes the
> > device and frees the devm-allocated resources (e.g. *info).
> > 
> > So letting aside that some resources are never freed after a failed call
> > of scmi_remove(), the user of the scmi node will probably stumble over
> > accessing freed memory soon. I wouldn't be surprised if that was
> > exploitable.
> 
> At first, I thought that this was only some kind of mess that needed to
> be cleaned up in the remove/exit path but without any real way of being
> exposed: this *bad* assumption of mine came from the fact that the SCMI
> drivers (like scmi-cpufreq scmi-hwmon) register to use the core SCMI stack
> services using an exported symbol (scmi_register_driver) and then, additionally,
> each SCMI driver willing to use some the core SCMI protocols issues a
> try_module_get (since the protocols could have been modularized)
> 
> So that in a module scenario (with scmi-module being the core platform
> driver:
> 
> root@deb-buster-arm64:~# lsmod 
> Module                  Size  Used by
> scmi_hwmon             16384  0
> scmi_module           131072  2 scmi_hwmon
> 
> root@deb-buster-arm64:~# rmmod ./scmi-module.ko 
> rmmod: ERROR: Module scmi_module is in use by: scmi_hwmon
> 
> BUT, then I realized that all of this refcounting is not going to save me
> from an explicitly triggered unbind:
> 
> root@deb-buster-arm64:~# echo 'firmware:scmi' > /sys/bus/platform/drivers/arm-scmi/unbind 
> [11601.798671] arm-scmi firmware:scmi: remove callback returned a non-zero value. This will be ignored.
> 
> This will indeed expose the issue you reported.
> 
> > I quickly tried to fix this issue, but didn't understand the driver good
> > enough. I think a fix would involve adding a get_device() call to
> > scmi_handle_get() to prevent scmi_remove() being called while a handle
> > exists.
> > 
> 
> I tried to address the issue with the get_device() calls as you
> suggested, but it has really no effect and in fact looking at the code
> on the remove path there is nothing that seems to be taken into
> consideration to stop a removal (and this indeed is consistent with the
> fact that any error returned on the removal/exit path is effectively ignored).

Hmm, I thought get_device() would delay calling .remove() but indeed it
only delays kobject_release().

> What instead DOES seem to work, it is to add a proper devlink with
> device_link_add() declaring any SCMI driver device (like from cpufreq)
> as a consumer of the core stack platform device: this will NOT stop the
> removal, BUT it does trigger an implicit unbind at first of all the SCMI
> drivers currently loaded when an unbind is requested for the core SCMI
> platform device: as a consequence the removal can now be triggered
> safely by an unbind even if some SCMI driver users are present (they
> will be unbound too)

Yeah, that should work. Not sure if there is an easier construct.

> Now, for this cleanup to fully work, some (long due) rework of the SCMI
> core devices handling and removal is needed (and it has partially
> already started in other unrelated series), so my plan would be to, at
> first, immediately add a '.suppress_bind_attrs = true' to the core driver
> to mitigate this (which is also easily portable to stable) and then
> rework properly the core SCMI removeal routines to safely re-enable
> a proper unbind. (or not re-enabling it if deemed not worth BUT anyway
> reworking the core SCMI remove/exit path)
> 
> I'll check my plan with Sudeep, any thoughts from you about this ?
> Am I missing something more ?

Sure, but nothing I'd be aware of :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel