linux-kernel - Re: [PATCH 0/1] uprobes: Kill __replace_page(), change uprobe_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFxkW2cFB3us0_A+Nq04UYOq4H_ta5rNYyuYyZd=YOuk2w@mail.gmail.com>
Date:	Mon, 9 Dec 2013 18:08:43 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Andi Kleen <andi@...stfloor.org>,
	Borislav Petkov <bp@...en8.de>,
	Hugh Dickins <hughd@...gle.com>,
	Ingo Molnar <mingo@...nel.org>, Jiri Kosina <jkosina@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/1] uprobes: Kill __replace_page(), change
 uprobe_write_opcode() to rely on gup(WRITE)

On Mon, Dec 9, 2013 at 1:18 PM, Oleg Nesterov <oleg@...hat.com> wrote:
>
> It is not clear to me if Linus still dislikes this change or not.
> Let me send the patch "officially" so that it can be nacked if I
> misunderstood the result of discussion.

So quite frankly, if the caller guarantees that it has looked up the
vma, and verified it, then I'm ok with it.

But in that case, I think you should pass in the vma as an argument,
and verify it for now.  Because quite frankly, the reason I reacted to
this all is that it is NOT AT ALL obvious that the callers actually do
that.

That uprobe_write_opcode() also only works if the instruction is
within a single page, another thing that it doesn't actually check.
And again, it is not at all obvious by looking at the callers that
that check has ever been done. The interface looks like you can just
rewrite an arbitrary byte range using that function.

In other words: looking at that function, my immediate reaction is
still "it's buggy". Why? Because it seems to make assumptions about
the callers that are never checked and are not at all obvious that
they have ever *been* checked. In fact, callers can clearly call that
thing without ever even looking up the vma (which it must to do check
that it's not shared), and we look it up *again* by using that slow
nasty get_user_pages() thing.

Also, quite frankly, I think the routine is still just horrible. If I
read it right, even after that cleanup you do a page table walk
*THREE* times:

 - 2x get_user_pages()
 - page_check_address()

and that's without the whole "retry" thing. It all seems pretty damn
pointless. Wouldn't it be better to do it *once*, and then the retry
logic is for "oops, we need to cow the page we looked up, so we need
to drop the page table lock and allocate a new page".

So I'd actually prefer this to (a) pass in the vma (to make it obvious
that the caller must look it up!) (b) add a few sanity checks (not
page-crossing) and I think possibly (c) make it a bit smarter still.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/