linux-kernel - Re: [PATCH v7 1/3] riscv: Avoid unaligned access when relocating modules

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: <CAJM55Z-v0EwrZp876DdLSzad2u55nJV_uBs_+MUJDqFW5MTPkA@mail.gmail.com>
Date:   Tue, 31 Oct 2023 06:11:47 -0700
From:   Emil Renner Berthing <emil.renner.berthing@...onical.com>
To:     Charlie Jenkins <charlie@...osinc.com>,
        linux-riscv@...ts.infradead.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Cc:     Eric Biederman <ebiederm@...ssion.com>,
        Kees Cook <keescook@...omium.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Andreas Schwab <schwab@...ux-m68k.org>,
        Emil Renner Berthing <emil.renner.berthing@...onical.com>,
        Samuel Holland <samuel.holland@...ive.com>,
        Nelson Chu <nelson@...osinc.com>,
        Emil Renner Berthing <kernel@...il.dk>
Subject: Re: [PATCH v7 1/3] riscv: Avoid unaligned access when relocating modules

Charlie Jenkins wrote:
> From: Emil Renner Berthing <kernel@...il.dk>
>
> With the C-extension regular 32bit instructions are not
> necessarily aligned on 4-byte boundaries. RISC-V instructions
> are in fact an ordered list of 16bit little-endian
> "parcels", so access the instruction as such.
>
> This should also make the code work in case someone builds
> a big-endian RISC-V machine.
>
> Signed-off-by: Emil Renner Berthing <kernel@...il.dk>
> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
> ---
>  arch/riscv/kernel/module.c | 153 +++++++++++++++++++++++----------------------
>  1 file changed, 77 insertions(+), 76 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 7c651d55fcbd..a9e94e939cb5 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -27,68 +27,86 @@ static bool riscv_insn_valid_32bit_offset(ptrdiff_t val)
>  #endif
>  }
>
> -static int apply_r_riscv_32_rela(struct module *me, u32 *location, Elf_Addr v)
> +static int riscv_insn_rmw(void *location, u32 keep, u32 set)
> +{
> +	u16 *parcel = location;
> +	u32 insn = (u32)le16_to_cpu(parcel[0]) | (u32)le16_to_cpu(parcel[1]) << 16;
> +
> +	insn &= keep;
> +	insn |= set;
> +
> +	parcel[0] = cpu_to_le32(insn);

Why cpu_to_le32(insn)? Unless I've misunderstood something downcasting unsigned
to unsigned values in C (eg. from u32 to u16) is defined to always discard the
most signifcant bits, so cpu_to_le16(insn) should be fine.

> +	parcel[1] = cpu_to_le16(insn >> 16);
> +	return 0;
> +}
> +
> +static int riscv_insn_rvc_rmw(void *location, u16 keep, u16 set)
> +{
> +	u16 *parcel = location;
> +
> +	*parcel = cpu_to_le16((le16_to_cpu(*parcel) & keep) | set);

In this case, maybe consider writing it out like above just so it's easy to see
that the two functions does the same just for differently sized instructions.
The compiler should generate the same code.

> +	return 0;
> +}
> +
> ...