linux-kernel - Re: [PATCH 3/3 v2] i2c: i801: enable irq for byte_by

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: <20120626182452.18804d77@endymion.delvare>
Date:	Tue, 26 Jun 2012 18:24:52 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Daniel Kurtz <djkurtz@...omium.org>
Cc:	ben-linux@...ff.org, seth.heasley@...el.com, ben@...adent.org.uk,
	David.Woodhouse@...el.com, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, olofj@...omium.org,
	bleung@...omium.org
Subject: Re: [PATCH 3/3 v2] i2c: i801: enable irq for byte_by_byte 
 transactions

Hi Daniel,

On Wed, 20 Jun 2012 15:34:49 +0200, Jean Delvare wrote:
> I'll get at least the I2C block read tested on ICH5.

Some more notes about this patch, now that have done some testing...

> > (...)
> > +		} else if (priv->count < priv->len - 1) {
> 
> Is this just paranoia or do you believe it could actually happen? I
> admit I don't see how. If it is paranoia then the same should be done
> for the read part.

This is good paranoia and I recommend doing the same for block reads,
where it is even more important. An array overrun on block write means
you'll be sending random memory bytes to your I2C device, which is
already bad. But an array overrun on block read means you'll _trash_
random memory bytes, with tragic consequences. The bug right below did
exactly this to me: the block read would never stop, and after a few
seconds only my machine would die with a different error each time,
depending on what memory got trashed.

I ended up testing for both count < len and len <= I2C_SMBUS_BLOCK_MAX,
for both reads and writes. Probably that's overkill and either should
be sufficient, but I wanted to play it really safe at first.

> 
> > +			outb(priv->data[++priv->count], SMBBLKDAT(priv));
> > +			outb_p(priv->cmd | I801_START, SMBHSTCNT(priv));
> 
> I don't get the I801_START here and above. We are in the middle of the block
> transaction. The polling-based code only sets I801_START at the
> beginning, not for every byte, so why would it be different here?

I confirm this is a serious bug. The patch broke I2C block read on my
ICH5 machine completely, until I removed these two I801_START.

After fixing this, testing is conclusive on my ICH5 machine (where the
SMBus interrupt is shared.) BTW, the debug message complaining when
pcists.ints isn't set should be dropped, in my case the interrupt is
shared with the sound chip and DVB-T card, and this caused a massive
flood of this message while I was debugging. The message isn't terribly
useful anyway IMHO, there's nothing wrong with that case.

Next step for me is testing on my ICH7-M laptop.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/