linux-kernel - Re: [GIT PULL] objtool/urgent for v5.15-rc4

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]
Date:   Sun, 3 Oct 2021 16:02:06 -0700
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Borislav Petkov <bp@...e.de>, Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Nathan Chancellor <nathan@...nel.org>, x86-ml <x86@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>
Subject: Re: [GIT PULL] objtool/urgent for v5.15-rc4

On Sun, Oct 03, 2021 at 12:10:38PM -0700, Linus Torvalds wrote:
> Replying to myself just to add more proper people to the cc.
> 
> I'm also wondering how I could possibly be the only person who saw the warning.
> 
> I don't think I am, and I think that people who signed off on commit
> 24ff65257375 ("objtool: Teach get_alt_entry() about more relocation
> types") and claimed to have "tested" it, clearly didn't actually do
> so.
> 
> PeterZ/Josh/Nathan: see the thread at
> 
>    https://lore.kernel.org/lkml/CAHk-=wiZwq-0LknKhXN4M+T8jbxn_2i9mcKpO+OaBSSq_Eh7tg@mail.gmail.com/
> 
> if you need more context, but I suspect you can figure it out just
> from this email too.

Sorry about that.  I think Peter and I failed to run this through
regression testing.  We can work on tightening up our process.

Definitely *not* Nathan's fault.  His 'Tested-by' only means that this
fixed his particular issue.

> > > Looking at the kvm code, that kvm_fastop_exception thing is some funky sh*t.
> > >
> > > I _think_ the problem is that 'kvm_fastop_exception' is done with bare
> > > asm at the top-level and that triggers some odd interaction with other
> > > section data, but I really don't know.
> >
> > No, it's the fact that it is marked as a global function (why?) that
> > it then causes problems.
> >
> > Now, I don't actually see why it would cause problems (the same way I
> > don't see why it's marked global). But removing that
> >
> >      ".global kvm_fastop_exception \n"
> >
> > works.
> >
> > I suspect it makes the linker do the relocation for us before objtool
> > runs, because now that it's a local name, there is no worry about
> > multiply defined symbols of the same name or anything like that.
> >
> > I also suspect that the reason for the warning is that the symbol type
> > has never been declared, so it's not marked as a STT_FUNC in the
> > relocation information.

Right, basically objtool's complaining that it doesn't know how to
handle the NOTYPE symbol.  But really, any non-object symbol should be
straightforward.  I may just remove these warnings altogether in favor
of something much simpler (something like the patch below).

> > In the meantime, I think the exception handling for kvm
> > divide/multiply emulation is badly broken right now. Hmm?

The warning is harmless, so it doesn't necessarily mean anything's
broken.  That said, I have no idea what's going in that code or why
kvm_fastop_exception() is clearing %esi.


diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index f58ecc50fb10..0217ac3fa7ff 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -58,22 +58,11 @@ void __weak arch_handle_alternative(unsigned short feature, struct special_alt *
 {
 }
 
-static bool reloc2sec_off(struct reloc *reloc, struct section **sec, unsigned long *off)
+static void reloc_to_sec_off(struct reloc *reloc, struct section **sec,
+			     unsigned long *off)
 {
-	switch (reloc->sym->type) {
-	case STT_FUNC:
-		*sec = reloc->sym->sec;
-		*off = reloc->sym->offset + reloc->addend;
-		return true;
-
-	case STT_SECTION:
-		*sec = reloc->sym->sec;
-		*off = reloc->addend;
-		return true;
-
-	default:
-		return false;
-	}
+	*sec = reloc->sym->sec;
+	*off = reloc->sym->offset + reloc->addend;
 }
 
 static int get_alt_entry(struct elf *elf, struct special_entry *entry,
@@ -109,11 +98,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		WARN_FUNC("can't find orig reloc", sec, offset + entry->orig);
 		return -1;
 	}
-	if (!reloc2sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off)) {
-		WARN_FUNC("don't know how to handle reloc symbol type: %s",
-			   sec, offset + entry->orig, orig_reloc->sym->name);
-		return -1;
-	}
+	reloc_to_sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off);
 
 	if (!entry->group || alt->new_len) {
 		new_reloc = find_reloc_by_dest(elf, sec, offset + entry->new);
@@ -131,11 +116,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		if (arch_is_retpoline(new_reloc->sym))
 			return 1;
 
-		if (!reloc2sec_off(new_reloc, &alt->new_sec, &alt->new_off)) {
-			WARN_FUNC("don't know how to handle reloc symbol type: %s",
-				  sec, offset + entry->new, new_reloc->sym->name);
-			return -1;
-		}
+		reloc_to_sec_off(new_reloc, &alt->new_sec, &alt->new_off);
 
 		/* _ASM_EXTABLE_EX hack */
 		if (alt->new_off >= 0x7ffffff0)