Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030810AbbD1Ryc (ORCPT ); Tue, 28 Apr 2015 13:54:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48596 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030324AbbD1Ry3 (ORCPT ); Tue, 28 Apr 2015 13:54:29 -0400 Date: Tue, 28 Apr 2015 12:54:28 -0500 From: Josh Poimboeuf To: Petr Mladek Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Michal Marek , Peter Zijlstra , x86@kernel.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] x86, stackvalidate: Compile-time stack frame pointer validation Message-ID: <20150428175428.GA29346@treble.redhat.com> References: <0c54981fcba75d6a32ad4074786b99bbf0fc0810.1430142416.git.jpoimboe@redhat.com> <20150428164414.GG21044@dhcp128.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150428164414.GG21044@dhcp128.suse.cz> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3151 Lines: 101 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/