Unreviewed code in 3.11

Kernel development, like development in most free software projects, is built around the concept of peer review. All patches should be reviewed by at least one other developer; that, it is hoped, will catch bugs before they are merged and lead to a higher-quality end result. While a lot of code review does take place in the kernel project, it is also clearly the case that a certain amount of code goes in without ever having been looked at by anybody other than the original developer. A couple of recent episodes bear a closer look; they show why the community values code review and the hazards of skipping it.

O_TMPFILE

The O_TMPFILE option to the open() system call was pulled into the mainline during the 3.11 merge window; prior to that pull, it had not been posted in any public location. There is no doubt that it provides a useful feature; it allows an application to open a file in a given filesystem with no visible name. In one stroke, it does away with a whole range of temporary file vulnerabilities, most of which are based on guessing which name will be used. O_TMPFILE can also be used with the linkat() system call to create a file and make it visible in the filesystem, with the right permissions, in a single atomic step. There can be no doubt that application developers will want to make good use of this functionality once it becomes widely available.

That said, O_TMPFILE has been going through a bit of a rough start. It did not take long for Linus to express concerns about the new API; in short, there was no way for applications to determine that they were running on a system where O_TMPFILE was not supported. A couple of patches later, those issues had been addressed. Since then, a couple of bugs have been found in the implementation; one, fixed by Zheng Liu, would oops the kernel. Another, reported by Andy Lutomirski, corrupts the underlying filesystem through the creation of a bogus inode. Finally, few filesystems actually support this new option at this point, so it is not something that developers can count on having available, even on Linux systems.

Meanwhile, Christoph Hellwig has questioned the API chosen for this feature:

Christoph suggests that it would have been better to create a new tmpfile() system call rather than adding this feature to open(). In the end, he has said, O_TMPFILE needs some more time:

Neither Al Viro (the author of this feature) nor Linus has responded to Christoph's suggestions, leading one to believe that the current plan is to go ahead with the current implementation. Once the O_TMPFILE ABI is exposed in the 3.11 release, it will need to be supported indefinitely. It certainly is supportable in its current form, but it may well have come out better with a bit more discussion prior to merging.

Secret security fixes

Russell King's pre-3.11-rc4 pull request does not appear to have been sent to any public list. Based on the merge commit in the mainline, what Russell said about this request was:

Evidently, the fact that eight out of the 22 commits in that request were security fixes does not constitute a "real overall theme." The patches seem like worthwhile hardening for the ARM architecture, evidently written in response to disclosures made at the recently concluded Black Hat USA 2013 event. While most of the patches carry an Acked-by from Nicolas Pitre, none of them saw any kind of public review before heading into the mainline.

It was not long before Olof Johansson encountered a number of problems with the changes, leading to several systems that were unable to boot. LWN reader kalvdans pointed out a different obvious bug in the code. Olof suggested that, perhaps, the patches might have benefited from some time in the linux-next repository, but Russell responded:

In this case, it is far from clear that much was gained by taking these patches out of the normal review process. The list of distributors rushing to deploy these fixes to users prior to their public disclosure is likely to be quite short, and, in any case, the cure, as was merged for 3.11-rc4, was worse than the disease. As of this writing, neither bug has been fixed in the mainline, though patches exist for both.

That said, one can certainly imagine scenarios where it might make sense to develop and merge a fix outside of public view. If a security vulnerability is known to be widely exploitable, one wants to get the fix as widely distributed as possible before the attackers are able to develop their exploits. In many cases, though, the vulnerabilities are not readily exploitable, or, as is the case for the bulk of deployed ARM systems, there is no way to quickly distribute an update in any case. In numerous other cases, the vulnerability in question has been known to the attacker community for a long time before it comes to the attention of a kernel developer.

For all of those cases, chances are high that the practice of developing fixes in secret does more harm than good. As has been seen here, such fixes can introduce bugs of their own; sometimes, those new bugs can be new security problems as well. In other situations, as in the O_TMPFILE case, unreviewed code also runs the risk of introducing suboptimal APIs that must then be maintained for many years. The code review practices we have developed over the years exist for a reason; bypassing those practices introduces a whole new set of risks to the kernel development process. The 3.11 development cycle has demonstrated just how real those risks can be.

---

(Log in to post comments)