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

[Josh Poimboeuf]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/