linux-kernel - RE: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers that use struct wmi

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: <MN0PR12MB6101441AA29CE815DCAD26AEE27B9@MN0PR12MB6101.namprd12.prod.outlook.com>
Date:   Thu, 1 Sep 2022 21:39:37 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@....com>
To:     Hans de Goede <hdegoede@...hat.com>
CC:     Mark Gross <markgross@...nel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: RE: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
 that use struct wmi_driver

[Public]



> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@...il.com>
> Sent: Thursday, September 1, 2022 12:17
> To: Limonciello, Mario <Mario.Limonciello@....com>
> Cc: Hans de Goede <hdegoede@...hat.com>; Mark Gross
> <markgross@...nel.org>; Platform Driver <platform-driver-
> x86@...r.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>
> Subject: Re: [PATCH v2] platform/x86: wmi: Allow duplicate GUIDs for drivers
> that use struct wmi_driver
> 
> On Mon, Aug 29, 2022 at 11:20 PM Mario Limonciello
> <mario.limonciello@....com> wrote:
> >
> > The WMI subsystem in the kernel currently tracks WMI devices by
> > a GUID string not by ACPI device.  The GUID used by the `wmi-bmof`
> > module however is available from many devices on nearly every machine.
> >
> > This originally was though to be a bug, but as it happens on most
> 
> thought
> 
> > machines it is a design mistake.  It has been fixed by tying an ACPI
> > device to the driver with struct wmi_driver. So drivers that have
> > moved over to struct wmi_driver can actually support multiple
> > instantiations of a GUID without any problem.
> >
> > Add an allow list into wmi.c for GUIDs that the drivers that are known
> > to use struct wmi_driver.  The list is populated with `wmi-bmof` right
> > now. The additional instances of that in sysfs with be suffixed with -%d
> 
> ...
> 
> > +/* allow duplicate GUIDs as these device drivers use struct wmi_driver */
> > +static const char * const allow_duplicates[] = {
> > +       "05901221-D566-11D1-B2F0-00A0C9062910", /* wmi-bmof */
> > +       NULL,
> 
> No comma for the terminator.
> 
> > +};
> 
> ...
> 
> > +static int guid_count(const guid_t *guid)
> > +{
> > +       struct wmi_block *wblock;
> > +       int count = 0;
> > +
> > +       list_for_each_entry(wblock, &wmi_block_list, list) {
> > +               if (guid_equal(&wblock->gblock.guid, guid))
> > +                       count++;
> > +       }
> > +
> > +       return count;
> > +}
> 
> I haven't deeply checked the code, but this kind of approach is
> fragile and proven to be error prone as shown in practice. The
> scenario is (again, not sure if it's possible, need a comment in the
> code if it's not possible) removing an entry from the list in the
> middle and trying to add it again. you will see the duplicate count
> values. That's why in the general case we use IDA or similar
> approaches.

It shouldn't be possible to add/remove from the list, they're fixed
lists that were parsed from _WDG.

Hans - since you already took this into your review queue, can you
land fixes for the 3 things Andy pointed out before it goes to -next
or do you want me to do a manual follow up for them?

1) Spelling error in commit message
2) Remove comma on terminator
3) Add a comment why guid_count is safe (if you agree with me it is)