LWN: Comments on "The "rare write" mechanism"

The "rare write" mechanism - attribution

jospoortvliet — Mon, 26 Jun 2017 09:33:49 +0000

I believe that that wasn't disputed, Kees properly credited. The article, however, seems to suggest he wrote the code, merely inspired by the PAX feature, which is clearly not the case.

The "rare write" mechanism - attribution

minipli — Tue, 06 Jun 2017 18:53:29 +0000

It's a "poor rip-off" simply because Kees' code is broken. It introduces a Dirty COW alike bug and, ironically enough, this time brought to you by the team making Linux "more secure" -- well, not quite, in this case. ;)

This is just a prime example that they don't understand what they're copying. How would such an approach gain Linux security in any sensible way? It's always the bloody details that count -- that little corner case that also needs to be taken care of to make a security feature complete. But so far KSPP has just copy-pasted PaX/grsec code without fully understanding the interconnections with other features and code changes. I'd say, they need to slow down and start reading the whole thing, trying to understand what they just read and repeat -- until no new "Ahhh!" moments appear any more. What happens now isn't much more than a grep 'n sed over the patch without understanding what those hunks really do -- what other changes they need the grep didn't catch.

The "rare write" mechanism - attribution

likryol — Tue, 06 Jun 2017 17:32:34 +0000

Why can't you (validly) point out that your code is easily available without attacking people? Your stance here is right, but the tone and delivery (on both sides of the argument to be fair) is why there is friction.

The "rare write" mechanism - attribution

nix — Tue, 06 Jun 2017 11:24:11 +0000

You say "a poor rip-off with minor bikeshedding": anyone else would say "changes of the sort routinely needed for upstreaming which PaXTeam is unwilling to do". If that makes a "poor rip-off", then the *entire kernel* is a "poor rip-off", which makes it odd that grsecurity is based on it. (This is a general problem with the grsecurity position: if the kernel is so all-around terrible and run by thieves, scoundrels, liars, incompetents and scum as they constantly claim, why on earth are they using it as a basis for their work? Perhaps because it's not anywhere near as bad as all that after all. Further, if it's thievery to take the grsecurity work and modify and upstream it, why on earth is it not thievery for grsecurity to take the kernel and modify it, particularly if they then accompany the result with contracts that try to restrict its further distribution? I mean, yes, both are legally permissible, but if I look for scurrilous behaviour here I do not see it in the kernel maintainers. Oh noes, they're giving people's work a wide audience and all you have to do in exchange for this is act like a decent human being!)

The "rare write" mechanism - attribution

tao — Tue, 06 Jun 2017 11:21:42 +0000

"Based on PaX's x86 pax_{open,close}_kernel() implementation, this
allows HAVE_ARCH_RARE_WRITE to work on x86."

I find it hard to say that it lacks proper attribution though.

The "rare write" mechanism - attribution

minipli — Tue, 06 Jun 2017 11:05:57 +0000

The relevant code from the PaX Patch for v4.9.24 looks astonishingly similar to Kees' proposal, no?
So, now, please tell me this is not just a poor rip-off with minor bikeshedding applied!?

The "rare write" mechanism - attribution

PaXTeam — Tue, 06 Jun 2017 10:56:20 +0000

yeah man, makes one wonder where Kees could have possibly copied my code from if it was 'proprietary'. if you are unable to tell then Kees/LWN failed at properly attributing the origin of my code so complain to them instead of spewing ad hominem. of course, a smart cookie such as yourself could have also just looked at the test patch collection linked from, wait for it, the PaX homepage but then assuming that you failed to do that so that you can justify going on an irrelevant diatribe would be bad faith, wouldn't it? ;)

The "rare write" mechanism - attribution

flussence — Tue, 06 Jun 2017 03:17:53 +0000

I can understand how something like that might be overlooked. If grsecurity wasn't de-facto proprietary anyone could do a diff instead of having to assume paxteam is once again acting in bad faith.

The "rare write" mechanism - attribution

jospoortvliet — Mon, 05 Jun 2017 07:00:05 +0000

I can't judge the merits either but when I read the article my impression is that most of the code is written by Cook, merely inspired by the work pax did. If, in reality, 80-90% is copied from PAX and the patch is essentially a port and perhaps clean-up of PAX code to the current kernel I suggest the wording is indeed flawed.

The "rare write" mechanism - attribution

giraffedata — Sun, 04 Jun 2017 00:44:37 +0000

If the code is largely copied from the PaX/grsecurity patch set, saying that Cook proposed a mechanism based on functionality (or function) in that patch set is inadequate. That says to me original code that does something similar to what the PaX/grsecurity code does.

If it's basically code copied with some renaming, and Cook said so in the talk as PaXTeam says, then "based on code in the PaX/grsecurity patch set" or "derived from code ..." would be less misleading, if not, "largely copied from ..."

The "rare write" mechanism

minipli — Sat, 03 Jun 2017 15:52:57 +0000

That might work, however, I don't see any such changes in the proposed patch.

The "rare write" mechanism

mm7323 — Fri, 02 Jun 2017 20:10:07 +0000

Shouldn't the nmi handler just save and clear, then restore the state of X86_CR0_WP or what ever architecture specific register to close that specific worry?

The "rare write" mechanism

Tara_Li — Fri, 02 Jun 2017 15:03:28 +0000

I'm thinking that "rare" in this case might be specifying more the ratio of writes to reads - something like a webpage with comments where the webpage is read a large number of times relative to the comments made to it - so it might make sense to just re-write the page as a static HTML page to add comments, rather than doing massive database look-ups each time the article is presented to users. Of course the viability of this depends on the ratio between time spent reading and time spent writing - so maybe a range of solutions is needed such as write_1_in_100(), write_1_in_10000(), write_1_in_1m(), etc.

The "rare write" mechanism

nurh — Fri, 02 Jun 2017 14:07:45 +0000

I did misread the intent of your LKML post, and I apologize. We've corrected the error. Thank you for the clarification.

The "rare write" mechanism

madscientist — Fri, 02 Jun 2017 13:02:16 +0000

No one is talking about copyright infringement (which is good, since that's not a valid claim). What's being discussed is attribution, and in particular attribution in the article (not attribution in the patches). I think the comments section of the article is a valid place to discuss that. I have no opinion on the merits.

The "rare write" mechanism

ballombe — Fri, 02 Jun 2017 10:09:35 +0000

Just write 'The implementation' instead of 'Cook implementation' etc.

The "rare write" mechanism

itvirta — Fri, 02 Jun 2017 07:02:36 +0000

> the code is mine, not his.

I think there might be more appropriate forums for copyright infringement claims than comments on a news site.

The "rare write" mechanism

jeff_marshall — Fri, 02 Jun 2017 01:39:15 +0000

PaXTeam,

The article seems to point this out in a straightforward (to me) manner:

> Kees Cook proposed such a mechanism based on similar functionality that exists in the PaX/grsecurity patch set

What would you propose that the LWN editors do differently?

The "rare write" mechanism

PaXTeam — Fri, 02 Jun 2017 01:08:02 +0000

Dear Jon,

you can't say you weren't warned. some specific examples of apparently wilfully misattributed authorship claims:

> Cook's implementation contains architecture-specific code that relies on CPU features on x86 and ARM that selectively
> enable and disable write access to areas of memory.

the x86 specific code is mine, not his.

> Cook noted that his code is inlined [...]

again, it's my code which he copied.

> Cook's newly introduced __wr_rare annotation[...]

it's my __read_only attribute.

> Cook gave a simple example of the usage of the single rare_write() call by converting a function in net/core/sock_diag.c from:

it's my code too.

> Cook's implementation of the rare write functions on x86 became the following:

it's all my code.

The "rare write" mechanism

corbet — Fri, 02 Jun 2017 00:25:27 +0000

Dear PaX Team: The provenance of the code in question is clearly described in the article itself. It is, meanwhile, development work that is relevant to the kernel development community and worthy of coverage. No bullshit, no plagiarism. No stopping.

The "rare write" mechanism

PaXTeam — Fri, 02 Jun 2017 00:22:22 +0000

Dear LWN editors,

can we please stop with these bullshit plagiarism based articles? most if not all of the code attributed to Kees in the article is basically mine (he said so himself in the very post you linked to), pointless renaming and introduced bugs aside.

The "rare write" mechanism

minipli — Thu, 01 Jun 2017 21:39:32 +0000

The article only shows the x86 specific helpers and is missing the arch agnostic primitives. However, those are required to understand my concerns. Let me quote them here for completeness:

static __always_inline rare_write_begin(void)
{
preempt_disable();
local_irq_disable();
barrier();
__arch_rare_write_begin();
barrier();
}

static __always_inline rare_write_end(void)
{
barrier();
__arch_rare_write_end();
barrier();
local_irq_enable();
preempt_enable_no_resched();
}

As can be seen, the rare_write_begin() primitive disables preemption and IRQ delivery on the local CPU. (The latter is a bug, actually, as the code could be called from a section that already had interrupts disabled (think of spin_lock_irqsave() et al.) and would wrongly re-enable them in rare_write_end(). But that's just another flaw of that code I don't want to focus on too much.) The point is, the local_irq_disable() doesn't prevent NMIs from interrupting a rare_write_begin()...rare_write_end() block. The NMI handler would run with CR0.WP disabled and, in turn, would allow an attacker to exploit this situation as mechanisms like COW won't work any more. So I raised that concern, as mentioned in the article, but I didn't question the implementation in PaX! In fact, well, let me just quote my email here as well:

"""
Well, doesn't look good to me. NMIs will still be able to interrupt
this code and will run with CR0.WP = 0.

Shouldn't you instead question yourself why PaX can do it "just" with
preempt_disable() instead?!
"""

As I'm not a native speaker my understanding of the second sentence might differ from yours, but, what I wanted to say was: PaX's implementation doesn't need to disable interrupts, it does handle the NMI case (and IRQ case, for that matter) perfectly fine and has a working COW semantic even then. I wasn't questioning PaX's implementation, I was questioning Kees' proposal. In fact, my question was asking *him* why PaX can have such a simple implementation of the rare_write_begin() helper (called pax_open_kernel() in the PaX patch) that is correct while his is not. (Hint: He'd missed a few hunks while copy-paste-bike-shedding.) But that led nowhere, just to Thomas arguing CR0.WP is a bad arch hack to begin with and that it shouldn't be used at all. I didn't quite agree, though.