| Message ID | 20210428222534.GJ3122264@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ics932s401: fix broken handling of errors when word reading fails | expand |
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 ... > +++ b/drivers/misc/ics932s401.c > @@ -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; > + temp = 0; > data->regs[regs_to_copy[i]] = temp >> 8; > } 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? But then we'd have to actually check sensors_valid in functions like calculate_src_freq, and i just don't know if it's worthwhile. Why not just revert this patch?
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. :) > > +++ b/drivers/misc/ics932s401.c > > @@ -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; > > + temp = 0; > > data->regs[regs_to_copy[i]] = temp >> 8; > > } > > 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". 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? > But then we'd have to actually check sensors_valid in functions like > calculate_src_freq, and i just don't know if it's worthwhile. Why not > just revert this patch? I had half expected them all to get reverted immediately, but since 5.12 went out with this still included, I thought it worth pointing out that despite UMN claims that none of their junk patches made it to Linus, this (mostly benign) one did. Granted, maybe 18 Jan 2019 was earlier than that, but who knows and who cares? :P --D
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?
On Wed, Apr 28, 2021 at 06:03:51PM -0700, Darrick J. Wong wrote: > I had half expected them all to get reverted immediately, but since 5.12 > went out with this still included, I thought it worth pointing out that > despite UMN claims that none of their junk patches made it to Linus, > this (mostly benign) one did. Granted, maybe 18 Jan 2019 was earlier > than that, but who knows and who cares? :P The claim was none of their "hypocrite commits" made it to Linus. That said nothing about any of their other patches that had been developed using some of their other research efforts. Greg isn't planning on sending any of the reverts until the 5.13 merge window, after doing a lot of reviews to determine which of the 190 commits were actually incorrect, and of those, how many may have actually introduced security vulnerabilities. "Good faith hypocrite commits", if you will. (Hey, we're all human; I know I've sent my share of buggy commits where I unintentionally introduced a bug. :-) If they can look at the buggy-yet-accepted commits, and map them to the research efforts in their previous papers, and then do feature analysis on the bad commits, maybe it will be possible for them to rework their "hypocrite commit" paper, and perhaps give us some insights about how to better find buggy commits in our code reviews --- that is, besides "try harder" and changing the Code of Conduct to prohibit intentionally introducing bugs (as they had proposed in their now-withdrawn paper). Also of interest is of the 68 UMN commits that did not cleanly revert; it may have been because they were incorrect, but were later fixed and/or reverted. In which case, we can probably learn about how long it takes for problems introduced by "good faith hypocrite commits" to get fixed naturally, without needing to do an emergency code review of all UMN patches sent in the past three years or so. - Ted
On Wed, Apr 28, 2021 at 03:25:34PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > 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... > > Fixes: b05ae01fdb89 ("misc/ics932s401: Add a missing check to i2c_smbus_read_word_data") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > drivers/misc/ics932s401.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/misc/ics932s401.c b/drivers/misc/ics932s401.c > index 2bdf560ee681..0f9ea75b0b18 100644 > --- a/drivers/misc/ics932s401.c > +++ b/drivers/misc/ics932s401.c > @@ -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; > + temp = 0; > data->regs[regs_to_copy[i]] = temp >> 8; > } > Many thanks for looking at this again, I'll add it to my series of patches for "reviewing all the crap and fixing it up" that I will be working to get merged for 5.13-final. greg k-h
diff --git a/drivers/misc/ics932s401.c b/drivers/misc/ics932s401.c index 2bdf560ee681..0f9ea75b0b18 100644 --- a/drivers/misc/ics932s401.c +++ b/drivers/misc/ics932s401.c @@ -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; + temp = 0; data->regs[regs_to_copy[i]] = temp >> 8; }