During warm reset device->fw_client is set to NULL. If a bus driver is
registered after this NULL setting and before new firmware clients are
enumerated by ISHTP, kernel panic will result in the function
ishtp_cl_bus_match(). This is because of reference to
device->fw_client->props.protocol_name.
ISH firmware after getting successfully loaded, sends a warm reset
notification to remove all clients from the bus and sets
device->fw_client to NULL. Until kernel v5.15, all enabled ISHTP kernel
module drivers were loaded after any of the first ISHTP device was
registered, regardless of whether it was a matched or an unmatched
device. This resulted in all drivers getting registered much before the
warm reset notification from ISH. Starting kernel v5.16, this issue got
exposed after the change was introduced to load only bus drivers for the
respective matching devices. In this scenario, cros_ec_ishtp device and
cros_ec_ishtp driver are registered after the warm reset
device_fw_client NULL setting. cros_ec_ishtp driver_register() triggers
the callback to ishtp_cl_bus_match() to match driver to the device and
causes kernel panic in guid_equal() when dereferencing fw_client NULL
pointer to get protocol_name.
Fixes: f155dfeaa4ee ("platform/x86: isthp_eclite: only load for matching devices")
Fixes: facfe0a4fdce ("platform/chrome: chros_ec_ishtp: only load for matching devices")
Fixes: 0d0cccc0fd83 ("HID: intel-ish-hid: hid-client: only load for matching devices")
Fixes: 44e2a58cb880 ("HID: intel-ish-hid: fw-loader: only load for matching devices")
Signed-off-by: Tanu Malhotra <[email protected]>
Tested-by: Shaunak Saha <[email protected]>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 81385ab37fa9..4f540906268f 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -241,8 +241,8 @@ static int ishtp_cl_bus_match(struct device *dev, struct device_driver *drv)
struct ishtp_cl_device *device = to_ishtp_cl_device(dev);
struct ishtp_cl_driver *driver = to_ishtp_cl_driver(drv);
- return guid_equal(&driver->id[0].guid,
- &device->fw_client->props.protocol_name);
+ return(device->fw_client ? guid_equal(&driver->id[0].guid,
+ &device->fw_client->props.protocol_name) : 0);
}
/**
--
2.34.1
On Sun, 2023-03-26 at 20:23 -0700, Tanu Malhotra wrote:
some minor nits below.
> During warm reset device->fw_client is set to NULL. If a bus driver
> is
> registered after this NULL setting and before new firmware clients
> are
> enumerated by ISHTP, kernel panic will result in the function
> ishtp_cl_bus_match(). This is because of reference to
> device->fw_client->props.protocol_name.
>
> ISH firmware after getting successfully loaded, sends a warm reset
> notification to remove all clients from the bus and sets
> device->fw_client to NULL. Until kernel v5.15, all enabled ISHTP
> kernel
> module drivers were loaded after any of the first ISHTP device was
> registered, regardless of whether it was a matched or an unmatched
> device. This resulted in all drivers getting registered much before
> the
> warm reset notification from ISH. Starting kernel v5.16, this issue
> got
> exposed after the change was introduced to load only bus drivers for
> the
> respective matching devices.
One paragraph break will be better.
> In this scenario, cros_ec_ishtp device and
> cros_ec_ishtp driver are registered after the warm reset
> device_fw_client NULL setting. cros_ec_ishtp driver_register()
> triggers
> the callback to ishtp_cl_bus_match() to match driver to the device
> and
> causes kernel panic in guid_equal() when dereferencing fw_client NULL
> pointer to get protocol_name.
>
> Fixes: f155dfeaa4ee ("platform/x86: isthp_eclite: only load for
> matching devices")
> Fixes: facfe0a4fdce ("platform/chrome: chros_ec_ishtp: only load for
> matching devices")
> Fixes: 0d0cccc0fd83 ("HID: intel-ish-hid: hid-client: only load for
> matching devices")
> Fixes: 44e2a58cb880 ("HID: intel-ish-hid: fw-loader: only load for
> matching devices")
>
No need of blank line.
> Signed-off-by: Tanu Malhotra <[email protected]>
> Tested-by: Shaunak Saha <[email protected]>
You can also add
Acked-by: Srinivas Pandruvada <[email protected]>
Also add
Cc: <[email protected]> # 5.16+
> ---
When you submit "PATCH v2", Add change log here. (That is below "---").
Something like this:
v2
- Updated commit description
- Added CC to stable
> drivers/hid/intel-ish-hid/ishtp/bus.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index 81385ab37fa9..4f540906268f 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -241,8 +241,8 @@ static int ishtp_cl_bus_match(struct device *dev,
> struct device_driver *drv)
> struct ishtp_cl_device *device = to_ishtp_cl_device(dev);
> struct ishtp_cl_driver *driver = to_ishtp_cl_driver(drv);
>
> - return guid_equal(&driver->id[0].guid,
> - &device->fw_client->props.protocol_name);
> + return(device->fw_client ? guid_equal(&driver->id[0].guid,
> + &device->fw_client-
> >props.protocol_name) : 0);
Align the second line to char "d" after "(".
Thanks,
Srinivas
> }
>
> /**