linux-kernel - Re: [PATCH v2 05/12] powerpc: Do not dereference code as 'struct ppc_inst' (uprobe, code-patching, feature-fixups)

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]

Message-ID: <CACzsE9qw7CuZdBoSVVUjkcZe6Z8Vzy9iNDVE7E3JLhJER+Z9xw@mail.gmail.com>
Date:   Tue, 15 Jun 2021 13:41:22 +1000
From:   Jordan Niethe <jniethe5@...il.com>
To:     Christophe Leroy <christophe.leroy@...roup.eu>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v2 05/12] powerpc: Do not dereference code as 'struct
 ppc_inst' (uprobe, code-patching, feature-fixups)

On Thu, May 20, 2021 at 11:50 PM Christophe Leroy
<christophe.leroy@...roup.eu> wrote:
>
> 'struct ppc_inst' is an internal structure to represent an instruction,
> it is not directly the representation of that instruction in text code.
> It is not meant to map and dereference code.
>
> Dereferencing code directly through 'struct ppc_inst' has two main issues:
> - On powerpc, structs are expected to be 8 bytes aligned while code is
> spread every 4 byte.
> - Should a non prefixed instruction lie at the end of the page and the
> following page not be mapped, it would generate a page fault.
>
> In-memory code must be accessed with ppc_inst_read().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  arch/powerpc/kernel/uprobes.c     | 2 +-
>  arch/powerpc/lib/code-patching.c  | 8 ++++----
>  arch/powerpc/lib/feature-fixups.c | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
> index 186f69b11e94..46971bb41d05 100644
> --- a/arch/powerpc/kernel/uprobes.c
> +++ b/arch/powerpc/kernel/uprobes.c
> @@ -42,7 +42,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
>                 return -EINVAL;
>
>         if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> -           ppc_inst_prefixed(auprobe->insn) &&
> +           ppc_inst_prefixed(ppc_inst_read(&auprobe->insn)) &&
>             (addr & 0x3f) == 60) {
>                 pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n");
>                 return -EINVAL;
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 870b30d9be2f..0308429b0d1a 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -329,13 +329,13 @@ static unsigned long branch_iform_target(const struct ppc_inst *instr)
>  {
>         signed long imm;
>
> -       imm = ppc_inst_val(*instr) & 0x3FFFFFC;
> +       imm = ppc_inst_val(ppc_inst_read(instr)) & 0x3FFFFFC;
>
>         /* If the top bit of the immediate value is set this is negative */
>         if (imm & 0x2000000)
>                 imm -= 0x4000000;
>
> -       if ((ppc_inst_val(*instr) & BRANCH_ABSOLUTE) == 0)
> +       if ((ppc_inst_val(ppc_inst_read(instr)) & BRANCH_ABSOLUTE) == 0)
>                 imm += (unsigned long)instr;
>
>         return (unsigned long)imm;
> @@ -345,13 +345,13 @@ static unsigned long branch_bform_target(const struct ppc_inst *instr)
>  {
>         signed long imm;
>
> -       imm = ppc_inst_val(*instr) & 0xFFFC;
> +       imm = ppc_inst_val(ppc_inst_read(instr)) & 0xFFFC;
>
>         /* If the top bit of the immediate value is set this is negative */
>         if (imm & 0x8000)
>                 imm -= 0x10000;
>
> -       if ((ppc_inst_val(*instr) & BRANCH_ABSOLUTE) == 0)
> +       if ((ppc_inst_val(ppc_inst_read(instr)) & BRANCH_ABSOLUTE) == 0)
>                 imm += (unsigned long)instr;
>
>         return (unsigned long)imm;
> diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
> index fe26f2fa0f3f..8905b53109bc 100644
> --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -51,7 +51,7 @@ static int patch_alt_instruction(struct ppc_inst *src, struct ppc_inst *dest,
>
>         instr = ppc_inst_read(src);
>
> -       if (instr_is_relative_branch(*src)) {
> +       if (instr_is_relative_branch(ppc_inst_read(src))) {
The above variable instr could be used here, but that is not an issue
with this patch.
>                 struct ppc_inst *target = (struct ppc_inst *)branch_target(src);
>
>                 /* Branch within the section doesn't need translating */
> --
> 2.25.0
>
Reviewed by: Jordan Niethe <jniethe5@...il.com>