linux-kernel - Re: [PATCH 1/2] x86, stackvalidate: Compile-time stack frame pointer validation

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] [day] [month] [year] [list]

Message-ID: <20150428175428.GA29346@treble.redhat.com>
Date:	Tue, 28 Apr 2015 12:54:28 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Michal Marek <mmarek@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] x86, stackvalidate: Compile-time stack frame pointer
 validation

On Tue, Apr 28, 2015 at 06:44:15PM +0200, Petr Mladek wrote:
> On Mon 2015-04-27 08:56:27, Josh Poimboeuf wrote:
> > +		case 0x89:
> > +			insn_get_modrm(&insn);
> > +			if (insn.modrm.bytes[0] == 0xe5)
> > +				/* mov sp, bp */
> > +				mov++;
> > +			break;
> 
> I am a bit courious. You check 0x89 and 0x5e but I see the following
> in the disasembly:
> 
>        	   48 89 e5                mov    %rsp,%rbp
> 
> I wonder if 48 is just a prefix that is filtered by insn_get_opcode or
> what.
> 
> Sigh, I still need to learn a lot about assembly.

Yeah, 0x48 is a prefix which means the operands are 64-bit.  Here is a
useful, albeit very dense, x86 assembly reference:

  http://ref.x86asm.net/coder64.html


> > +		case 0xc3: /* ret */
> > +			ret++;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (push != 1 || mov != 1 || !pop || !ret || pop != ret) {
> > +		WARN("%s() is missing frame pointer logic.  Please use FUNC_ENTER.",
> > +		     func->name);
> 
> This looks quite weak condition to me. It accepts the instructions in
> any order and at any place.

It's really designed to determine whether the frame pointer is being
updated, with some basic sanity checks.  It doesn't guarantee that it's
being done _correctly_.  Which is ok I think, since all assembly code is
hand-coded by people who (hopefully) are being careful and know exactly
what they're doing.

And as it turns out, today, >99% of callable asm functions don't have
any frame pointer logic and will fail this check.

> Also livepatching will rely on having fentry before the prologue. Do
> you plan to add support for this ordering?

No, because this is only intended to analyze hand-crafted asm code,
which generally doesn't have the fentry call (and so live patching can't
patch it).

However, if we wanted to enable the patching of asm functions in the
future, we could consider adding the fentry call to the FUNC_ENTER macro
later on.

> > +	for (i = 0; i < sections_nr; i++) {
> > +		sec = malloc(sizeof(*sec));
> > +		if (!sec) {
> > +			perror("malloc");
> > +			return -1;
> > +		}
> > +		memset(sec, 0, sizeof(*sec));
> > +
> > +		INIT_LIST_HEAD(&sec->symbols);
> > +		INIT_LIST_HEAD(&sec->relas);
> > +
> > +		s = elf_getscn(elf->elf, i);
> > +		if (!s) {
> > +			perror("elf_getscn");
> > +			return -1;
> 
> This is leaking "sec". It is not added to the list, so you could not
> free it later. The same problem is on many other locations.

Ah, right.  It really doesn't matter much since this is a short running
userspace program, but I'll fix it up.

> > +	/* sanity check, one more call to elf_nextscn() should return NULL */
> > +	if (elf_nextscn(elf->elf, s)) {
> > +		WARN("section entry mismatch");
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I basically ended here. I was rather curious how such a thing could
> get implemented than doing a proper review.

Thanks a lot for looking!

-- 
Josh
--
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/