linux-kernel - Re: [PATCH 09/39] Annotate hardware config module parameters in drivers/i2c/

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:   Thu, 01 Dec 2016 14:12:52 +0000
From:   David Howells <dhowells@...hat.com>
To:     Jean Delvare <jdelvare@...e.de>, minyard@....org
Cc:     dhowells@...hat.com, linux-kernel@...r.kernel.org,
        gnomes@...rguk.ukuu.org.uk, Wolfram Sang <wsa@...-dreams.de>,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH 09/39] Annotate hardware config module parameters in drivers/i2c/

Jean Delvare <jdelvare@...e.de> wrote:

> > Note that we do still need to do the module initialisation because some
> > drivers have viable defaults set in case parameters aren't specified and
> > some drivers support automatic configuration (e.g. PNP or PCI) in addition
> > to manually coded parameters.
> 
> Initializing the driver when you are not able to honor the user request
> looks wrong to me. I don't see how some drivers having sane defaults
> justifies that. Using the defaults when no parameters are passed is one
> thing (good), still using the defaults when parameters are passed is
> another (bad), and you should be able to differentiate between these two
> cases.

Corey Minyard argues the other way:

	This would prevent any IPMI interface from working if any address was
	given on the kernel command line. I'm not sure what the best policy
	is, but that sounds like a possible DOS to me.

Your preference allows someone to prevent a driver from initialising - which
could also be bad.  The problem is that I don't think there's any way to do
both.

Note that the policy isn't actually handled in any of these patches, but will
be handled in a later patchset that is on top of my EFI changes also.

> > Suggested-by: One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
> 
> I know this is only a Suggested-by and not a Signed-off-by, but still I
> believe the Developer's Certificate of Origin applies, and it says:
> "using your real name (sorry, no pseudonyms or anonymous
> contributions.)"

I asked him what he prefers - but no response.

> No objection from me, but I think you missed several i2c bus driver
> parameters:
> 
> i2c-ali15x3.c:module_param(force_addr, ushort, 0);
> i2c-piix4.c:module_param (force_addr, int, 0);
> i2c-sis5595.c:module_param(force_addr, ushort, 0);
> i2c-viapro.c:module_param(force_addr, ushort, 0);

Okay, thanks.  They all seem to encode ioports.  All changed.

> And maybe the following ones, but I'm not sure if forcibly enabling a
> device is part of what you need to prevent:
> 
> i2c-piix4.c:module_param (force, int, 0);
> i2c-sis630.c:module_param(force, bool, 0);
> i2c-viapro.c:module_param(force, bool, 0);

I don't know either.  One could argue it *should* be locked down because its
need appears to reflect a BIOS bug.

David