linux-kernel - Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

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: <20160801153205.646a04df@endymion>
Date:	Mon, 1 Aug 2016 15:32:05 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:	Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host
 Notify is used

Hi Benjamin,

On Fri, 29 Jul 2016 11:12:12 +0200, Jean Delvare wrote:
> On Thu, 28 Jul 2016 11:50:39 +0200, Benjamin Tissoires wrote:
> > +static void i801_disable_host_notify(struct i2c_adapter *adapter)
> > +{
> > +	struct i801_priv *priv = i2c_get_adapdata(adapter);
> 
> You pass the adapter as the parameter, but don't need it. All you need
> is priv, which the caller has too. So you could pass priv as the
> parameter directly and avoid the glue code.
> 
> > +
> > +	if (!(priv->features & FEATURE_HOST_NOTIFY))
> > +		return;
> > +
> > +	/* disable Host Notify... */
> > +	outb_p(0, SMBSLVCMD(priv));
> 
> This assumes there's only one bit in the register, which is not true.
> There are 3 bits. I did not notice the problem during my original
> review, but in i801_enable_host_notify() you are silently zero-ing the
> other 2 bits too, which isn't nice. You should only touch the bit that
> matters to you, both here and in i801_enable_host_notify().

Thinking about it some more, I'd like to add: what if host notify was
enabled before the driver was loaded? The driver should leave the
controller in the same state it found it, ideally. We have learned in
the past that doing otherwise for PEC and I2C mode, for example, leads
to problems (lock-up on shutdown) on some systems.

This could happen with Host Notify as well. So I think you should save
the value at driver load time and restore it at unload time (same we do
with the hstcfg register.)

-- 
Jean Delvare
SUSE L3 Support