From: Darrick J. Wong <[email protected]>
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 <[email protected]>
---
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;
}
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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