| 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
| ||
|
Message-ID: <73b70bcd-1476-ebf7-f84e-d7b01b1e022e@gmail.com>
Date: Thu, 22 Aug 2019 21:18:01 +0100
From: Julien <julien.thierry.kdev@...il.com>
To: Raphael Gault <raphael.gault@....com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
jpoimboe@...hat.com
Cc: peterz@...radead.org, catalin.marinas@....com, will.deacon@....com,
raph.gault+kdev@...il.com
Subject: Re: [RFC v4 05/18] objtool: special: Adapt special section handling
Hi Raphaël,
On 16/08/19 13:23, Raphael Gault wrote:
> This patch abstracts the few architecture dependent tests that are
> perform when handling special section and switch tables. It enables any
> architecture to ignore a particular CPU feature or not to handle switch
> tables.
I think it would be better if this patch only focused on CPU features
and leave dealing with switch table to subsequent patches.
>
> Signed-off-by: Raphael Gault <raphael.gault@....com>
> ---
> tools/objtool/arch/arm64/Build | 1 +
> tools/objtool/arch/arm64/arch_special.c | 22 +++++++++++++++
> .../objtool/arch/arm64/include/arch_special.h | 10 +++++--
> tools/objtool/arch/x86/Build | 1 +
> tools/objtool/arch/x86/arch_special.c | 28 +++++++++++++++++++
> tools/objtool/arch/x86/include/arch_special.h | 9 ++++++
> tools/objtool/check.c | 24 ++++++++++++++--
> tools/objtool/special.c | 9 ++----
> tools/objtool/special.h | 3 ++
> 9 files changed, 96 insertions(+), 11 deletions(-)
> create mode 100644 tools/objtool/arch/arm64/arch_special.c
> create mode 100644 tools/objtool/arch/x86/arch_special.c
>
> diff --git a/tools/objtool/arch/arm64/Build b/tools/objtool/arch/arm64/Build
> index bf7a32c2b9e9..3d09be745a84 100644
> --- a/tools/objtool/arch/arm64/Build
> +++ b/tools/objtool/arch/arm64/Build
> @@ -1,3 +1,4 @@
> +objtool-y += arch_special.o
> objtool-y += decode.o
> objtool-y += orc_dump.o
> objtool-y += orc_gen.o
> diff --git a/tools/objtool/arch/arm64/arch_special.c b/tools/objtool/arch/arm64/arch_special.c
> new file mode 100644
> index 000000000000..a21d28876317
> --- /dev/null
> +++ b/tools/objtool/arch/arm64/arch_special.c
> @@ -0,0 +1,22 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "../../special.h"
> +#include "arch_special.h"
> +
> +void arch_force_alt_path(unsigned short feature,
> + bool uaccess,
> + struct special_alt *alt)
> +{
> +}
> diff --git a/tools/objtool/arch/arm64/include/arch_special.h b/tools/objtool/arch/arm64/include/arch_special.h
> index 63da775d0581..185103be8a51 100644
> --- a/tools/objtool/arch/arm64/include/arch_special.h
> +++ b/tools/objtool/arch/arm64/include/arch_special.h
> @@ -30,7 +30,13 @@
> #define ALT_ORIG_LEN_OFFSET 10
> #define ALT_NEW_LEN_OFFSET 11
>
> -#define X86_FEATURE_POPCNT (4 * 32 + 23)
> -#define X86_FEATURE_SMAP (9 * 32 + 20)
> +static inline bool arch_should_ignore_feature(unsigned short feature)
> +{
> + return false;
> +}
>
> +static inline bool arch_support_switch_table(void)
> +{
> + return false;
If I understand correctly , this gets later (patch 8) replaced by
arch_find_switch_table() which can return NULL if unable to find a
switch table.
So, is it necessary to introduce this function at this stage of the
patchset? Can't we just have directly arch_find_switch_table() ?
Also, I believe that in the end you keep the two
arch_support_switch_table() implementations (x86 and arm64) despite
getting rid of the only caller (in patch 8).
> +}
> #endif /* _ARM64_ARCH_SPECIAL_H */
> diff --git a/tools/objtool/arch/x86/Build b/tools/objtool/arch/x86/Build
> index 1f11b45999d0..63e167775bc8 100644
> --- a/tools/objtool/arch/x86/Build
> +++ b/tools/objtool/arch/x86/Build
> @@ -1,3 +1,4 @@
> +objtool-y += arch_special.o
> objtool-y += decode.o
> objtool-y += orc_dump.o
> objtool-y += orc_gen.o
> diff --git a/tools/objtool/arch/x86/arch_special.c b/tools/objtool/arch/x86/arch_special.c
> new file mode 100644
> index 000000000000..6583a1770bb2
> --- /dev/null
> +++ b/tools/objtool/arch/x86/arch_special.c
> @@ -0,0 +1,28 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +#include "../../special.h"
> +#include "arch_special.h"
> +
> +void arch_force_alt_path(unsigned short feature,
> + bool uaccess,
Maybe you could have something like:
void arch_select_alt_path(unsigned short feature, struct special_alt *alt);
or arch_process_alt_inst(...);
This way you just let the arch decide what to do for alternative
instructions associated with their features, and call it in the "if
(entry->feature)" branch of get_alt_entry().
You don't need to pass the "uaccess" as that info is globally accessible
from builtin.h.
> + struct special_alt *alt)
> +{
> + if (feature == X86_FEATURE_SMAP) {
> + if (uaccess)
> + alt->skip_orig = true;
> + else
> + alt->skip_alt = true;
> + }
> +}
> diff --git a/tools/objtool/arch/x86/include/arch_special.h b/tools/objtool/arch/x86/include/arch_special.h
> index 424ce47013e3..fce2b1193194 100644
> --- a/tools/objtool/arch/x86/include/arch_special.h
> +++ b/tools/objtool/arch/x86/include/arch_special.h
> @@ -33,4 +33,13 @@
> #define X86_FEATURE_POPCNT (4 * 32 + 23)
> #define X86_FEATURE_SMAP (9 * 32 + 20)
>
> +static inline bool arch_should_ignore_feature(unsigned short feature)
> +{
> + return feature == X86_FEATURE_POPCNT;
> +}
With my above suggestion you shouldn't need that and just use the arch
specific alternative handling set the alt->skip_orig.
> +
> +static inline bool arch_support_switch_table(void)
> +{
> + return true;
> +}
> #endif /* _X86_ARCH_SPECIAL_H */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 30e147391dcb..4af6422d3428 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -729,7 +729,7 @@ static int handle_group_alt(struct objtool_file *file,
> last_orig_insn = insn;
> }
>
> - if (next_insn_same_sec(file, last_orig_insn)) {
> + if (last_orig_insn && next_insn_same_sec(file, last_orig_insn)) {
> fake_jump = malloc(sizeof(*fake_jump));
> if (!fake_jump) {
> WARN("malloc failed");
> @@ -1061,6 +1061,26 @@ static struct rela *find_jump_table(struct objtool_file *file,
> table_rela = find_rela_by_dest(table_sec, table_offset);
> if (!table_rela)
> continue;
> + /*
> + * If we are on arm64 architecture, we now that we
> + * are in presence of a switch table thanks to
> + * the `br <Xn>` insn. but we can't retrieve it yet.
> + * So we just ignore unreachable for this file.
> + */
> + if (!arch_support_switch_table()) {
> + file->ignore_unreachables = true;
> + return NULL;
> + }
> +
> + rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
> + if (rodata_rela) {
> + /*
> + * Use of RIP-relative switch jumps is quite rare, and
> + * indicates a rare GCC quirk/bug which can leave dead
> + * code behind.
> + */
> + if (text_rela->type == R_X86_64_PC32)
> + file->ignore_unreachables = true;
>
> /*
> * Use of RIP-relative switch jumps is quite rare, and
> @@ -1864,7 +1884,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> insn = first;
> sec = insn->sec;
>
> - if (insn->alt_group && list_empty(&insn->alts)) {
> + if (!insn->visited && insn->alt_group && list_empty(&insn->alts)) {
> WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
> sec, insn->offset);
> return 1;
> diff --git a/tools/objtool/special.c b/tools/objtool/special.c
> index b8ccee1b5382..7a0092d6e5b3 100644
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -81,7 +81,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
> * feature path which is a "very very small percentage of
> * machines".
> */
> - if (feature == X86_FEATURE_POPCNT)
> + if (arch_should_ignore_feature(feature))
> alt->skip_orig = true;
>
> /*
> @@ -93,12 +93,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
> * find paths that see the STAC but take the NOP instead of
> * CLAC and the other way around.
> */
> - if (feature == X86_FEATURE_SMAP) {
> - if (uaccess)
> - alt->skip_orig = true;
> - else
> - alt->skip_alt = true;
> - }
> + arch_force_alt_path(feature, uaccess, alt);
> }
>
> orig_rela = find_rela_by_dest(sec, offset + entry->orig);
> diff --git a/tools/objtool/special.h b/tools/objtool/special.h
> index 35061530e46e..90626a7e41cf 100644
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/special.h
> @@ -27,5 +27,8 @@ struct special_alt {
> };
>
> int special_get_alts(struct elf *elf, struct list_head *alts);
> +void arch_force_alt_path(unsigned short feature,
> + bool uaccess,
> + struct special_alt *alt);
>
> #endif /* _SPECIAL_H */
>
CHeers,
--
Julien Thierry
Powered by blists - more mailing lists