Intentionally buggy commits for fame

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 8:57 UTC (Thu) by shalem (subscriber, #4062)
Parent article: Intentionally buggy commits for fame—and papers

I don't believe the commit which Guenter Roeck linked to in this email actually is malicious / buggy.

Guenter wrote the following about this: "The author of commit c9c63915519b is Kangjie Lu, not some student, and I have to assume that it intentionally introduced a problem. That was the whole point of the exercise, wasn't it ?"

I believe this was more of a hypothetical question / example of how we (the kernel community) now need to question every commit from the UMN, then an example of an actual malicious / buggy commit. At least I've gone over the commit twice and I cannot find anything wrong with it.

With that said, the entire situation still is a big mess and UMN's behavior here is inexcusable.

to post comments

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 9:13 UTC (Thu) by dvrabel (subscriber, #9500) [Link] (3 responses)

You can't tell the commit is buggy by just looking at the commit itself as it lacks sufficient context.

It returns without releasing a mutex. See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/...

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 9:24 UTC (Thu) by shalem (subscriber, #4062) [Link] (2 responses)

Ah, thank you for pointing that out.

That does look like an innocent mistake though. The error condition under which the return with the lock held would happen cannot be controlled by an attacker (I believe it is an I2C read which fails) and even if it could at worse it would be a DOS attach AFAICT. Unless I'm again missing something ?

So buggy yes, malicious no. But now we still get to wonder if the buggy-ness was intentional or just an honest mistake <sigh>.

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 10:32 UTC (Thu) by jani (subscriber, #74547) [Link]

> So buggy yes, malicious no. But now we still get to wonder if the buggy-ness was intentional or just an honest mistake <sigh>.

"Never attribute to malice that which is adequately explained by stupidity."
- https://en.wikipedia.org/wiki/Hanlon%27s_razor

"The Underhanded C Contest is a programming contest to turn out code that is malicious, but passes a rigorous inspection, and looks like an honest mistake even if discovered."
- https://en.wikipedia.org/wiki/Underhanded_C_Contest

We tend to err on the side of honest mistakes, and it can be abused.

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 22:28 UTC (Thu) by pbonzini (subscriber, #60935) [Link]

That's as much a failure of the C language as it is a failure of patch review.

Using a language without RAII is inexcusable.

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 9:22 UTC (Thu) by danielthompson (subscriber, #97243) [Link]

> I don't believe the commit which Guenter Roeck linked to in this email actually
> is malicious / buggy.

There is certainly a bug in that it introduces a new return path and that new return path does not release a mutex.

Whether this is malicious is more or less impossible to say. However it does seem reasonable to view with suspicion a buggy patch from a researcher studying ways to covertly introduce and mature "immature vulnerabilities" by hiding them in innocent looking patches.

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 9:25 UTC (Thu) by bronson (subscriber, #4806) [Link] (2 responses)

The first chunk is a useless change. Just churn, no improvement. I don’t know about the second chunk but I’d wager it’s an example of adding impossible-to-hit error checking that they’re now famous for.

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 10:22 UTC (Thu) by excors (subscriber, #95769) [Link] (1 responses)

It looks like the 'useless' first chunk was added by Guenter Roeck to tidy up the code - the commit message says "[groeck: One variable for return values is enough]", and the original patch from Kangjie Lu (https://www.mail-archive.com/linux-kernel@vger.kernel.org...) didn't have that change.

The second chunk looks like it's addressing a real problem that's not impossible to hit - lm80_read_value can return negative error codes and that wasn't being handled. Maybe that's only possible if there's a bizarre hardware failure (I2C/SMBus devices shouldn't just stop responding at arbitrary times) but it seems good practice for the kernel to handle those situations properly. (But the patch fixes the problem wrongly, because it returns without unlocking the mutex.)

Intentionally buggy commits for fame—and papers

Posted Apr 22, 2021 14:06 UTC (Thu) by bronson (subscriber, #4806) [Link]

Great investigation, I would have lost the wager.

Intentionally buggy commits for fame—and papers

Posted Apr 23, 2021 8:35 UTC (Fri) by andy_shev (subscriber, #75870) [Link]

The point is that maintainers always have to question all commits under their area of interest. This research proves that it’s not always the case.