linux-kernel - Re: [RFC PATCH v1] platform/x86: wmi: Do not register driver with invalid GUID

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <efe4b91f-2602-2115-738e-bb99b42ec5b6@redhat.com>
Date:   Wed, 26 Jul 2023 10:45:59 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Barnabás Pőcze <pobrn@...tonmail.com>,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        Mark Gross <markgross@...nel.org>, Armin Wolf <W_Armin@....de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [RFC PATCH v1] platform/x86: wmi: Do not register driver with
 invalid GUID

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@...tonmail.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;
>