diff mbox series

[RFC,v1] platform/x86: wmi: Do not register driver with invalid GUID

Message ID 20230715211604.1272227-1-pobrn@protonmail.com
State New
Headers
Series [RFC,v1] platform/x86: wmi: Do not register driver with invalid GUID |

Commit Message

Barnabás Pőcze July 15, 2023, 9:24 p.m. UTC 
  Since a WMI driver's ID table contains strings it is relatively
easy to make mistakes. At the moment, there is no feedback
if any of the specified GUIDs are invalid (since
028e6e204ace1f080cfeacd72c50397eb8ae8883).

So check if the GUIDs in the driver's ID table are valid,
print all invalid ones, and refuse to register the driver
if any of the GUIDs are invalid.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 drivers/platform/x86/wmi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Andy Shevchenko July 17, 2023, 9:49 a.m. UTC | #1 
On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:
> Since a WMI driver's ID table contains strings it is relatively
> easy to make mistakes. At the moment, there is no feedback
> if any of the specified GUIDs are invalid (since
> 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> 
> So check if the GUIDs in the driver's ID table are valid,
> print all invalid ones, and refuse to register the driver
> if any of the GUIDs are invalid.

Besides using wrong API (uuid_*() vs. guid_*() one), I don't
think we need to validate it here. Why not in file2alias.c?
Barnabás Pőcze July 17, 2023, 11:23 a.m. UTC | #2 
Hi


2023. július 17., hétfő 11:49 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:

On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:
> > Since a WMI driver's ID table contains strings it is relatively
> > easy to make mistakes. At the moment, there is no feedback
> > if any of the specified GUIDs are invalid (since
> > 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> >
> > So check if the GUIDs in the driver's ID table are valid,
> > print all invalid ones, and refuse to register the driver
> > if any of the GUIDs are invalid.
> 
> Besides using wrong API (uuid_*() vs. guid_*() one), I don't

As far as I can see `guid_parse()` also uses `uuid_is_valid()`, the format is the same.


> think we need to validate it here. Why not in file2alias.c?
> [...]

1) that seems like a more complicated change (duplicating `uuid_is_valid()`?);
2) that will only check the GUIDs specified by `MODULE_DEVICE_TABLE()`.

Arguably the second point is not that significant since most users will indeed
use `MODULE_DEVICE_TABLE()`. But I think the first point has some merit. And
furthermore, I think this check should be here regardless of whether file2alias.c
also contains an equivalent/similar check.


Regards,
Barnabás Pőcze
Andy Shevchenko July 17, 2023, 11:31 a.m. UTC | #3 
On Mon, Jul 17, 2023 at 11:23:50AM +0000, Barnabás Pőcze wrote:
> 2023. július 17., hétfő 11:49 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:
> On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:
> > > Since a WMI driver's ID table contains strings it is relatively
> > > easy to make mistakes. At the moment, there is no feedback
> > > if any of the specified GUIDs are invalid (since
> > > 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> > >
> > > So check if the GUIDs in the driver's ID table are valid,
> > > print all invalid ones, and refuse to register the driver
> > > if any of the GUIDs are invalid.
> > 
> > Besides using wrong API (uuid_*() vs. guid_*() one), I don't
> 
> As far as I can see `guid_parse()` also uses `uuid_is_valid()`, the format is the same.

Then add guid_is_valid() to complete the API. Perhaps with the renaming the
common part to something else.

> > think we need to validate it here. Why not in file2alias.c?
> > [...]
> 
> 1) that seems like a more complicated change (duplicating `uuid_is_valid()`?);
> 2) that will only check the GUIDs specified by `MODULE_DEVICE_TABLE()`.
> 
> Arguably the second point is not that significant since most users will indeed
> use `MODULE_DEVICE_TABLE()`. But I think the first point has some merit. And
> furthermore, I think this check should be here regardless of whether file2alias.c
> also contains an equivalent/similar check.

Why do we need it? We never match against wrong GUID from ACPI, since it would
be very weird ACPI table.

For file2alias what we would need is to split uuid.c in the kernel to something
like uuid.c and libuuid.c where the latter will be used as a C module in both
kernel and user space (i.o.w. to be compiled twice and linked accordingly).
Barnabás Pőcze July 19, 2023, 7:23 p.m. UTC | #4 
Hi


2023. július 17., hétfő 13:31 keltezéssel, Andy Shevchenko írta:

> On Mon, Jul 17, 2023 at 11:23:50AM +0000, Barnabás Pőcze wrote:
> > 2023. július 17., hétfő 11:49 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:
> > On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:
> > > > Since a WMI driver's ID table contains strings it is relatively
> > > > easy to make mistakes. At the moment, there is no feedback
> > > > if any of the specified GUIDs are invalid (since
> > > > 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> > > >
> > > > So check if the GUIDs in the driver's ID table are valid,
> > > > print all invalid ones, and refuse to register the driver
> > > > if any of the GUIDs are invalid.
> > >
> > > Besides using wrong API (uuid_*() vs. guid_*() one), I don't
> >
> > As far as I can see `guid_parse()` also uses `uuid_is_valid()`, the format is the same.
> 
> Then add guid_is_valid() to complete the API. Perhaps with the renaming the
> common part to something else.

But that would be the exact same function. GUIDs are UUIDs, aren't they?


> 
> > > think we need to validate it here. Why not in file2alias.c?
> > > [...]
> >
> > 1) that seems like a more complicated change (duplicating `uuid_is_valid()`?);
> > 2) that will only check the GUIDs specified by `MODULE_DEVICE_TABLE()`.
> >
> > Arguably the second point is not that significant since most users will indeed
> > use `MODULE_DEVICE_TABLE()`. But I think the first point has some merit. And
> > furthermore, I think this check should be here regardless of whether file2alias.c
> > also contains an equivalent/similar check.
> 
> Why do we need it? We never match against wrong GUID from ACPI, since it would
> be very weird ACPI table.
> [...]

The point is to catch typos in drivers' WMI ID tables.


Regards,
Barnabás Pőcze
Andy Shevchenko July 20, 2023, 8:36 a.m. UTC | #5 
On Wed, Jul 19, 2023 at 07:23:37PM +0000, Barnabás Pőcze wrote:
> 2023. július 17., hétfő 13:31 keltezéssel, Andy Shevchenko írta:
> > On Mon, Jul 17, 2023 at 11:23:50AM +0000, Barnabás Pőcze wrote:
> > > 2023. július 17., hétfő 11:49 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:
> > > On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:

...

> > > > Besides using wrong API (uuid_*() vs. guid_*() one), I don't
> > >
> > > As far as I can see `guid_parse()` also uses `uuid_is_valid()`, the format is the same.
> > 
> > Then add guid_is_valid() to complete the API. Perhaps with the renaming the
> > common part to something else.
> 
> But that would be the exact same function. GUIDs are UUIDs, aren't they?

Yes and no. If we want to validate the respective bit for GUID vs. UUID, they
will be different. Currently they are the same as validation is relaxed in the
kernel.

> > > > think we need to validate it here. Why not in file2alias.c?
> > > > [...]
> > >
> > > 1) that seems like a more complicated change (duplicating `uuid_is_valid()`?);
> > > 2) that will only check the GUIDs specified by `MODULE_DEVICE_TABLE()`.
> > >
> > > Arguably the second point is not that significant since most users will indeed
> > > use `MODULE_DEVICE_TABLE()`. But I think the first point has some merit. And
> > > furthermore, I think this check should be here regardless of whether file2alias.c
> > > also contains an equivalent/similar check.
> > 
> > Why do we need it? We never match against wrong GUID from ACPI, since it would
> > be very weird ACPI table.
> > [...]
> 
> The point is to catch typos in drivers' WMI ID tables.

Yes, that's what file2alias is for. We trust modules we build, right?
If you don't trust, then we have much bigger problem than this patch
tries to address.
Barnabás Pőcze July 20, 2023, 10:42 a.m. UTC | #6 
Hi


2023. július 20., csütörtök 10:36 keltezéssel, Andy Shevchenko írta:

> On Wed, Jul 19, 2023 at 07:23:37PM +0000, Barnabás Pőcze wrote:
> > 2023. július 17., hétfő 13:31 keltezéssel, Andy Shevchenko írta:
> > > On Mon, Jul 17, 2023 at 11:23:50AM +0000, Barnabás Pőcze wrote:
> > > > 2023. július 17., hétfő 11:49 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:
> > > > On Sat, Jul 15, 2023 at 09:24:16PM +0000, Barnabás Pőcze wrote:
> 
> ...
> 
> > > > > Besides using wrong API (uuid_*() vs. guid_*() one), I don't
> > > >
> > > > As far as I can see `guid_parse()` also uses `uuid_is_valid()`, the format is the same.
> > >
> > > Then add guid_is_valid() to complete the API. Perhaps with the renaming the
> > > common part to something else.
> >
> > But that would be the exact same function. GUIDs are UUIDs, aren't they?
> 
> Yes and no. If we want to validate the respective bit for GUID vs. UUID, they
> will be different. Currently they are the same as validation is relaxed in the
> kernel.

I see. Regardless, that is the only check `guid_parse()` does, so I don't think
it is unreasonable to have only that check for the time being.


> 
> > > > > think we need to validate it here. Why not in file2alias.c?
> > > > > [...]
> > > >
> > > > 1) that seems like a more complicated change (duplicating `uuid_is_valid()`?);
> > > > 2) that will only check the GUIDs specified by `MODULE_DEVICE_TABLE()`.
> > > >
> > > > Arguably the second point is not that significant since most users will indeed
> > > > use `MODULE_DEVICE_TABLE()`. But I think the first point has some merit. And
> > > > furthermore, I think this check should be here regardless of whether file2alias.c
> > > > also contains an equivalent/similar check.
> > >
> > > Why do we need it? We never match against wrong GUID from ACPI, since it would
> > > be very weird ACPI table.
> > > [...]
> >
> > The point is to catch typos in drivers' WMI ID tables.
> 
> Yes, that's what file2alias is for. We trust modules we build, right?
> If you don't trust, then we have much bigger problem than this patch
> tries to address.
> [...]

It seems we have to agree to disagree then.


Regards,
Barnabás Pőcze
Andy Shevchenko July 21, 2023, 9:17 a.m. UTC | #7 
On Thu, Jul 20, 2023 at 10:42:29AM +0000, Barnabás Pőcze wrote:
> 2023. július 20., csütörtök 10:36 keltezéssel, Andy Shevchenko írta:
> > On Wed, Jul 19, 2023 at 07:23:37PM +0000, Barnabás Pőcze wrote:

...

> > Yes, that's what file2alias is for. We trust modules we build, right?
> > If you don't trust, then we have much bigger problem than this patch
> > tries to address.

> It seems we have to agree to disagree then.

I am confused. The parts are related to each other, you can't disagree on both.

You mean we have much bigger problem in the kernel or file2alias is the proper
place? I.o.w. which part you agree with and respectively disagree?
Hans de Goede July 26, 2023, 8:45 a.m. UTC | #8 
Hi Barnabás,

On 7/15/23 23:24, Barnabás Pőcze wrote:
> Since a WMI driver's ID table contains strings it is relatively
> easy to make mistakes. At the moment, there is no feedback
> if any of the specified GUIDs are invalid (since
> 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> 
> So check if the GUIDs in the driver's ID table are valid,
> print all invalid ones, and refuse to register the driver
> if any of the GUIDs are invalid.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Thank you for working on this!

About the do this here, vs do this in file2alias.c discussion,
we have many old style WMI drivers which are not covered by
the check you are adding for the new style WMI bus driver.

So I think having a check in file2alias.c would be a very good
thing to have. AFAICT that would also cause compile time
failures rather then the run-time errors your current approach
results in.

I think that having an additional check like the one which you
propose has some value too, even if it is just to cover drivers
which for some reason don't use `MODULE_DEVICE_TABLE()`, but IMHO
the most important check to have is a check in file2alias.c .

Regards,

Hans




> ---
>  drivers/platform/x86/wmi.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index a78ddd83cda0..bf0be40b418a 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1513,6 +1513,19 @@ static int acpi_wmi_probe(struct platform_device *device)
>  int __must_check __wmi_driver_register(struct wmi_driver *driver,
>  				       struct module *owner)
>  {
> +	bool any_id_invalid = false;
> +
> +	for (const struct wmi_device_id *id = driver->id_table; *id->guid_string; id++) {
> +		if (!uuid_is_valid(id->guid_string)) {
> +			pr_err("driver '%s' has invalid GUID: %s",
> +			       driver->driver.name, id->guid_string);
> +			any_id_invalid = true;
> +		}
> +	}
> +
> +	if (any_id_invalid)
> +		return -EINVAL;
> +
>  	driver->driver.owner = owner;
>  	driver->driver.bus = &wmi_bus_type;
>
Barnabás Pőcze July 27, 2023, 10:54 p.m. UTC | #9 
Hi


2023. július 26., szerda 10:45 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:


> [...]
> On 7/15/23 23:24, Barnabás Pőcze wrote:
> > Since a WMI driver's ID table contains strings it is relatively
> > easy to make mistakes. At the moment, there is no feedback
> > if any of the specified GUIDs are invalid (since
> > 028e6e204ace1f080cfeacd72c50397eb8ae8883).
> >
> > So check if the GUIDs in the driver's ID table are valid,
> > print all invalid ones, and refuse to register the driver
> > if any of the GUIDs are invalid.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> 
> Thank you for working on this!
> 
> About the do this here, vs do this in file2alias.c discussion,
> we have many old style WMI drivers which are not covered by
> the check you are adding for the new style WMI bus driver.
> 
> So I think having a check in file2alias.c would be a very good
> thing to have. AFAICT that would also cause compile time
> failures rather then the run-time errors your current approach
> results in.
> 
> I think that having an additional check like the one which you
> propose has some value too, even if it is just to cover drivers
> which for some reason don't use `MODULE_DEVICE_TABLE()`, but IMHO
> the most important check to have is a check in file2alias.c .

Okay... any tips on how to avoid copying `uuid_is_valid()`?

Another idea I had was that maybe `struct wmi_device_id::guid_string` needs to be
changed to be `guid_t` and then `GUID_INIT()` or something similar could be used
to initialize it. That way it is impossible to mess up the format. The only downside
I can see is that guid is no longer "grep-able".


Regards,
Barnabás Pőcze
Andy Shevchenko July 28, 2023, 10:02 a.m. UTC | #10 
On Thu, Jul 27, 2023 at 10:54:26PM +0000, Barnabás Pőcze wrote:
> 2023. július 26., szerda 10:45 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> > On 7/15/23 23:24, Barnabás Pőcze wrote:

...

> > I think that having an additional check like the one which you
> > propose has some value too, even if it is just to cover drivers
> > which for some reason don't use `MODULE_DEVICE_TABLE()`, but IMHO
> > the most important check to have is a check in file2alias.c .
> 
> Okay... any tips on how to avoid copying `uuid_is_valid()`?

I think I already told the rough design: we need to split uuid.c to three
files: libuuid.h, libuuid.c uuid.c and libuuid.c should be built twice:
once for uuid.c and once for file2alias.c. libuuid.h should contain the
definitions file2alias.c is using.  Something like that.

> Another idea I had was that maybe `struct wmi_device_id::guid_string` needs to be
> changed to be `guid_t` and then `GUID_INIT()` or something similar could be used
> to initialize it. That way it is impossible to mess up the format. The only downside
> I can see is that guid is no longer "grep-able".

Strictly speaking you may not do that because it's a (semi-)ABI.
Barnabás Pőcze July 28, 2023, 2:39 p.m. UTC | #11 
Hi


2023. július 28., péntek 12:02 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:

> On Thu, Jul 27, 2023 at 10:54:26PM +0000, Barnabás Pőcze wrote:
> > 2023. július 26., szerda 10:45 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> > > On 7/15/23 23:24, Barnabás Pőcze wrote:
> 
> ...
> 
> > > I think that having an additional check like the one which you
> > > propose has some value too, even if it is just to cover drivers
> > > which for some reason don't use `MODULE_DEVICE_TABLE()`, but IMHO
> > > the most important check to have is a check in file2alias.c .
> >
> > Okay... any tips on how to avoid copying `uuid_is_valid()`?
> 
> I think I already told the rough design: we need to split uuid.c to three
> files: libuuid.h, libuuid.c uuid.c and libuuid.c should be built twice:
> once for uuid.c and once for file2alias.c. libuuid.h should contain the
> definitions file2alias.c is using.  Something like that.

What is not clear at all to me is how includes should be handled. `uuid_is_valid()`
uses `isxdigit()`, which is found in different header files based on whether it is
a kernel or user space build.


> 
> > Another idea I had was that maybe `struct wmi_device_id::guid_string` needs to be
> > changed to be `guid_t` and then `GUID_INIT()` or something similar could be used
> > to initialize it. That way it is impossible to mess up the format. The only downside
> > I can see is that guid is no longer "grep-able".
> 
> Strictly speaking you may not do that because it's a (semi-)ABI.

Why is that the case?


Regards,
Barnabás Pőcze
Andy Shevchenko July 31, 2023, 7:52 p.m. UTC | #12 
On Fri, Jul 28, 2023 at 02:39:07PM +0000, Barnabás Pőcze wrote:
> 2023. július 28., péntek 12:02 keltezéssel, Andy Shevchenko <andriy.shevchenko@linux.intel.com> írta:
> > On Thu, Jul 27, 2023 at 10:54:26PM +0000, Barnabás Pőcze wrote:
> > > 2023. július 26., szerda 10:45 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta:
> > > > On 7/15/23 23:24, Barnabás Pőcze wrote:

...

> > > > I think that having an additional check like the one which you
> > > > propose has some value too, even if it is just to cover drivers
> > > > which for some reason don't use `MODULE_DEVICE_TABLE()`, but IMHO
> > > > the most important check to have is a check in file2alias.c .
> > >
> > > Okay... any tips on how to avoid copying `uuid_is_valid()`?
> > 
> > I think I already told the rough design: we need to split uuid.c to three
> > files: libuuid.h, libuuid.c uuid.c and libuuid.c should be built twice:
> > once for uuid.c and once for file2alias.c. libuuid.h should contain the
> > definitions file2alias.c is using.  Something like that.
> 
> What is not clear at all to me is how includes should be handled. `uuid_is_valid()`
> uses `isxdigit()`, which is found in different header files based on whether it is
> a kernel or user space build.

It may be conditional build or some other tricks (I haven't investigated
myself, though). Alternatively libuuid.c can be included in the other
C-file.

> > > Another idea I had was that maybe `struct wmi_device_id::guid_string` needs to be
> > > changed to be `guid_t` and then `GUID_INIT()` or something similar could be used
> > > to initialize it. That way it is impossible to mess up the format. The only downside
> > > I can see is that guid is no longer "grep-able".
> > 
> > Strictly speaking you may not do that because it's a (semi-)ABI.
> 
> Why is that the case?

As a developer of that idea you should prove that it won't break any of all
possible user configurations (for example, first that comes to my mind is
multi-version modules: when kernel is not signed, but it might be not the
case, you need to research and convince us that there will be no breakage).
diff mbox series

Patch

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index a78ddd83cda0..bf0be40b418a 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1513,6 +1513,19 @@  static int acpi_wmi_probe(struct platform_device *device)
 int __must_check __wmi_driver_register(struct wmi_driver *driver,
 				       struct module *owner)
 {
+	bool any_id_invalid = false;
+
+	for (const struct wmi_device_id *id = driver->id_table; *id->guid_string; id++) {
+		if (!uuid_is_valid(id->guid_string)) {
+			pr_err("driver '%s' has invalid GUID: %s",
+			       driver->driver.name, id->guid_string);
+			any_id_invalid = true;
+		}
+	}
+
+	if (any_id_invalid)
+		return -EINVAL;
+
 	driver->driver.owner = owner;
 	driver->driver.bus = &wmi_bus_type;