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]
Date:   Wed, 27 Dec 2017 23:58:50 -0500
From:   William Breathitt Gray <vilhelm.gray@...il.com>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Cameron <jic23@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2] gpio: winbond: add driver

On Wed, Dec 27, 2017 at 07:42:21PM +0100, Maciej S. Szmigiero wrote:
>On 27.12.2017 01:24, William Breathitt Gray wrote:
>> On Mon, Dec 25, 2017 at 03:48:16PM +0100, Maciej S. Szmigiero wrote:
>(..)
>>> All the existing ISA bus drivers seem to depend on CONFIG_ISA_BUS_API
>>> instead of selecting it but IMHO this is wrong because:
>>> 1) This Kconfig option doesn't really enable or disable any bus support
>>> but building of a library of some common boilerplate code.
>>> Libraries are normally selected by drivers needing them and only provided
>>> as an user-selectable option if there is a possibility that a out-of-tree
>>> module would need it,
>>>
>>> 2) On x86_64 this option (or rather, its parent option CONFIG_ISA_BUS)
>>> cannot be enabled without CONFIG_EXPERT,
>>>
>>> 3) This device isn't really a ISA bus device any more than, for example,
>>> a 8250 serial port or a PC-style parallel port and these don't need
>>> that an user explicitly enables "ISA bus support" in his kernel
>>> configuration.
>> 
>> I can see what you mean about selecting ISA_BUS_API rather than having
>> it as a dependency for drivers. Part of the reason I added the
>> CONFIG_EXPERT dependency for CONFIG_ISA_BUS -- as well as having
>> CONFIG_ISA_BUS_API be a dependency for the drivers themselves -- was to
>> hide the ISA-style drivers which blindly poke at I/O port addresses,
>> lest a niave user enable all available drivers and unintentionally brick
>> their system when the drivers execute.
>> 
>> I think there is still merit in masking dangerous drivers such as this,
>> since the expected behavior nowadays is for the driver to probe for the
>> device before poking at memory; since ISA-style communication lacks a
>> standard method of detecting devices in hardware, these devices
>> generally pose a danger when loaded by niave users.
>
>This driver accesses the same Super I/O chip as w83627ehf hwmon and
>w83627hf_wdt watchdog drivers.
>In addition to this, there are loads of other hwmon and watchdog drivers
>for x86 Super I/Os in the tree, most of them using the same probing and
>communication style.
>There are even existing GPIO drivers for some Super I/Os like gpio-it87
>and gpio-f7188x.
>
>None of these drivers need CONFIG_EXPERT to be selected.
>
>Also, CONFIG_EXPERT is described as "Configure standard kernel features"
>and that "[it] allows certain base kernel options and settings to be
>disabled or tweaked" for "specialized environments".
>Enabling this driver is not about changing "standard kernel feature" or
>a "base kernel option [or] setting".

I'm sorry, I didn't make it quite clear in my previous reply. I agree
with you that CONFIG_EXPERT shouldn't be necessary for this driver -- in
the end, a select ISA_BUS_API line should be all that's needed to have
ISA bus driver support for your driver.

My reference to the CONFIG_EXPERT option is for masking options related
to other ISA-style buses not commonly found in desktop systems. Devices
like the Super I/O chip wouldn't fall into this category since LPC is
pretty common and the relevant I/O port addresses are usually well
known. Ultimately, the Winbond GPIO driver should not need CONFIG_EXPERT
to be enabled in order to select ISA_BUS_API.

William Breathitt Gray

>> 
>> William Breathitt Gray
>
>Maciej Szmigiero

Powered by blists - more mailing lists