linux-kernel - Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in rv_s

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: <b85bcf1d-9467-4df6-da11-8f0b24165ada@huawei.com>
Date:   Thu, 27 Jul 2023 15:42:15 +0800
From:   Pu Lehui <pulehui@...wei.com>
To:     Luke Nelson <lukenels@...washington.edu>, <bpf@...r.kernel.org>
CC:     Luke Nelson <luke.r.nels@...il.com>,
        kernel test robot <lkp@...el.com>,
        Xi Wang <xi.wang@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>,
        Yonghong Song <yonghong.song@...ux.dev>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Björn Töpel <bjorn@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next] riscv/bpf: Fix truncated immediate warning in
 rv_s_insn



On 2023/7/27 10:49, Luke Nelson wrote:
> Sparse warns that a cast in rv_s_insn truncates bits from the constant
> 0x7ff to 0xff.  The warning originates from the use of a constant offset
> of -8 in a store instruction in bpf_jit_comp64.c:
> 
>    emit(rv_sd(RV_REG_SP, -8, RV_REG_RA), &ctx);
> 
> rv_sd then calls rv_s_insn, with imm11_0 equal to (u16)(-8), or 0xfff8.
> 
> Here's the current implementation of rv_s_insn:
> 
>    static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
>    {
>            u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
> 
>            return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
>                   (imm4_0 << 7) | opcode;
>    }
> 
> imm11_0 is a signed 12-bit immediate offset of the store instruction. The
> instruction encoding requires splitting the immediate into bits 11:5 and
> bits 4:0. In this case, imm11_0 >> 5 = 0x7ff, which then gets truncated
> to 0xff when cast to u8, causing the warning from sparse. However, this is
> not actually an issue because the immediate offset is signed---truncating
> upper bits that are all set to 1 has no effect on the value of the
> immediate.
> 
> There is another subtle quirk with this code, which is imm11_5 is
> supposed to be the upper 7 bits of the 12-bit signed immediate, but its
> type is u8 with no explicit mask to select out only the bottom 7 bits.
> This happens to be okay here because imm11_5 is the left-most field in
> the instruction and the "extra" bit will be shifted out when imm11_5 is
> shifted left by 25.
> 
> This commit fixes the warning by changing the type of imm11_5 and imm4_0
> to be u32 instead of u8, and adding an explicit mask to compute imm11_5
> instead of relying on truncation + shifting.
> 
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202307260704.dUElCrWU-lkp@intel.com/
> In-Reply-To: <202307260704.dUElCrWU-lkp@...el.com>
> Signed-off-by: Luke Nelson <luke.r.nels@...il.com>
> Cc: Xi Wang <xi.wang@...il.com>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> ---
>   arch/riscv/net/bpf_jit.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/net/bpf_jit.h b/arch/riscv/net/bpf_jit.h
> index 2717f5490428..e159c6e3ff43 100644
> --- a/arch/riscv/net/bpf_jit.h
> +++ b/arch/riscv/net/bpf_jit.h
> @@ -238,7 +238,7 @@ static inline u32 rv_i_insn(u16 imm11_0, u8 rs1, u8 funct3, u8 rd, u8 opcode)
> 
>   static inline u32 rv_s_insn(u16 imm11_0, u8 rs2, u8 rs1, u8 funct3, u8 opcode)
>   {
> -	u8 imm11_5 = imm11_0 >> 5, imm4_0 = imm11_0 & 0x1f;
> +	u32 imm11_5 = (imm11_0 >> 5) & 0x7f, imm4_0 = imm11_0 & 0x1f;

Hi Luke,

keep u8 and add 0x7f explicit mask should work. I ran the repro case and 
it can silence the warning.

>
>   	return (imm11_5 << 25) | (rs2 << 20) | (rs1 << 15) | (funct3 << 12) |
>   		(imm4_0 << 7) | opcode;
> --
> 2.34.1
> 
>