Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <efe4b91f-2602-2115-738e-bb99b42ec5b6@redhat.com>
Date:   Wed, 26 Jul 2023 10:45:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [RFC PATCH v1] platform/x86: wmi: Do not register driver with
 invalid GUID
Content-Language: en-US, nl
To:     =?UTF-8?Q?Barnab=c3=a1s_P=c5=91cze?= <pobrn@protonmail.com>,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        Mark Gross <markgross@kernel.org>, Armin Wolf <W_Armin@gmx.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
References: <20230715211604.1272227-1-pobrn@protonmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230715211604.1272227-1-pobrn@protonmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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;
>