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: <20210429015406.GE1847222@casper.infradead.org>
Date:   Thu, 29 Apr 2021 02:54:06 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>, pakki001@....edu,
        gregkh@...uxfoundation.org, arnd@...db.de
Subject: Re: [PATCH] ics932s401: fix broken handling of errors when word
 reading fails

On Wed, Apr 28, 2021 at 06:03:51PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 28, 2021 at 11:46:24PM +0100, Matthew Wilcox wrote:
> > On Wed, Apr 28, 2021 at 03:25:34PM -0700, Darrick J. Wong wrote:
> > > In commit b05ae01fdb89, someone tried to make the driver handle i2c read
> > > errors by simply zeroing out the register contents, but for some reason
> > > left unaltered the code that sets the cached register value the function
> > > call return value.
> > > 
> > > The original patch was authored by a member of the Underhanded
> > > Mangle-happy Nerds, I'm not terribly surprised.  I don't have the
> > > hardware anymore so I can't test this, but it seems like a pretty
> > > obvious API usage fix to me...
> > 
> > Not sure why you cc'd linux-fsdevel, but that's how i got to see it ...
> 
> I whacked the wrong mutt shortcut key. :)

"A computer lets you make more mistakes faster than any other invention
with the possible exceptions of handguns and Tequila."

> > Looking at a bit more context in this function, shouldn't we rather clear
> > 'sensors_valid'?  or does it really make sense to pretend we read zero
> > (rather than 255) from this register?
> 
> Dunno.  As I said, I don't have that piece of hardware anymore.
> It probably does make more sense to fail the read or something, but
> since I can't QA it properly I'll go with "return a batch of zeroes".

It's from 2008 ... does anyone have that piece of hardware any more,
or should we delete the driver?  Seems like it's for use with the Intel
Pentium 4/D 955X chipset, which is from 2005.  Definitely out of support,
but I guess not entirely dead yet.

> Though ... if memory serves, the current behavior will probably shift
> the interesting parts of the errno code off the right end, filling the
> u8 buffer with all ones.  Maybe?

Right.  I mean, my smartwatch sometimes reads my heart rate as 255 bpm
when it gets cold.  I don't think they did QA at -40C.

But what's being read here is a bit more complex than beats-per-minute;
there's divisors and control registers and stuff.  I just don't feel
like '0' is a good fake value to pretend to have read.  I think we have
four options -- complicate the driver to make it understand that it
didn't read a value, pretend we read 0, 255 or the-last-value-we-read.
And the last option seems like the best to me?  So ...

@@ -134,7 +134,7 @@ static struct ics932s401_data *ics932s401_update_device(struct device *dev)
        for (i = 0; i < NUM_MIRRORED_REGS; i++) {
                temp = i2c_smbus_read_word_data(client, regs_to_copy[i]);
                if (temp < 0)
-                       data->regs[regs_to_copy[i]] = 0;
+                       continue;
                data->regs[regs_to_copy[i]] = temp >> 8;
        }
 

might be the best we can do?

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ