Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Apr 2021 14:47:47 -0500
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Sami Tolvanen <samitolvanen@google.com>
Cc:     x86@kernel.org, Kees Cook <keescook@chromium.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Sedat Dilek <sedat.dilek@gmail.com>,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
Subject: Re: [PATCH 02/15] objtool: Add CONFIG_CFI_CLANG support
Message-ID: <20210420194747.3snxvaaa4amfnbah@treble>
References: <20210416203844.3803177-1-samitolvanen@google.com>
 <20210416203844.3803177-3-samitolvanen@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210416203844.3803177-3-samitolvanen@google.com>
Precedence: bulk

On Fri, Apr 16, 2021 at 01:38:31PM -0700, Sami Tolvanen wrote:
> +static int fix_cfi_relocs(const struct elf *elf)
> +{
> +	struct section *sec, *tmpsec;
> +	struct reloc *reloc, *tmpreloc;
> +
> +	list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
> +		list_for_each_entry_safe(reloc, tmpreloc, &sec->reloc_list, list) {

These loops don't remove structs from the lists, so are the "safe"
iterators really needed?

> +			struct symbol *sym;
> +			struct reloc *func_reloc;
> +
> +			/*
> +			 * CONFIG_CFI_CLANG replaces function relocations to refer
> +			 * to an intermediate jump table. Undo the conversion so
> +			 * objtool can make sense of things.
> +			 */

I think this comment could be clearer if it were placed above the
function.

> +			if (!reloc->sym->sec->cfi_jt)
> +				continue;
> +
> +			if (reloc->sym->type == STT_SECTION)
> +				sym = find_func_by_offset(reloc->sym->sec,
> +							  reloc->addend);
> +			else
> +				sym = reloc->sym;
> +
> +			if (!sym || !sym->sec)
> +				continue;

This should be a fatal error, it probably means something's gone wrong
with the reading of the jump table.

> +
> +			/*
> +			 * The jump table immediately jumps to the actual function,
> +			 * so look up the relocation there.
> +			 */
> +			func_reloc = find_reloc_by_dest_range(elf, sym->sec, sym->offset, 5);

The jump instruction's reloc is always at offset 1, so this can maybe be
optimized to:

			func_reloc = find_reloc_by_dest(elf, sym->sec, sym->offset+1);

though of course that will probably break (future) arm64 objtool.  Maybe
an arch-specific offset from the start of the insn would be good.

> +			if (!func_reloc || !func_reloc->sym)
> +				continue;

This should also be an error.

> +
> +			reloc->sym = func_reloc->sym;

I think we should also do 'reloc->addend = 0', because of the
STT_SECTION case:

  0000000000000025  0000259e0000000b R_X86_64_32S           0000000000000000 .text..L.cfi.jumptable.8047 + 8

which then translates to

  0000000000000009  0000063200000004 R_X86_64_PLT32         0000000000000000 x2apic_prepare_cpu - 4

so the original addend of '8' is no longer valid.  Copying the '-4'
wouldn't be right either, because that only applies to a PLT32 text
reloc.  A '0' should be appropriate for the 32S data reloc.

This addend might not actually be read by any code, so it may not
currently be an actual bug, but better safe than sorry.

-- 
Josh