From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1041.google.com (mail-pj1-x1041.google.com [IPv6:2607:f8b0:4864:20::1041]) by sourceware.org (Postfix) with ESMTPS id D687F386196E for ; Sat, 26 Sep 2020 16:30:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D687F386196E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=palmer@dabbelt.com Received: by mail-pj1-x1041.google.com with SMTP id q4so1113441pjh.5 for ; Sat, 26 Sep 2020 09:30:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20150623.gappssmtp.com; s=20150623; h=date:subject:in-reply-to:cc:from:to:message-id:mime-version :content-transfer-encoding; bh=XWqw0y0y9MBRQIrmV39H6MxZvQ8tVy1CDj3H36uWsxs=; b=gAHwAo5QO+ChJqsDNy6u0cuEcluz68u3AopokJoV+L0K8YLBhoN7qhQul7WwerEjmh sTRO6L1fDS5jlvyYiI4GgwhOPwXLjlHxs/zRuKsJILzI3+78c5zrGMFOl7TtJcXRI43T 8C3YtTEPcb3PRYpSybmZIh46E5T3SxTT57JA3EyWQoooJ79IBQEq2fLUUTvXUdVbvY0s ftNyk8zFNDoMWa3yxms+HplRNLWmpbIXzLDnH7IdoRGkfsrLBD4EP0p1xr7aAYyyh6KF dGBTSEGWzEvTYysbrq0BJ3cVDeUjo3vC3Xtaa5fuM+GpE1X572bGMDyNLNS43aJuJNjF 3J9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:subject:in-reply-to:cc:from:to:message-id :mime-version:content-transfer-encoding; bh=XWqw0y0y9MBRQIrmV39H6MxZvQ8tVy1CDj3H36uWsxs=; b=dEv14es/y60I6wUMWBlRx8kUjaNh6Lpx2YSO4cMxc7mDS1NDxFSU56SpBo7biUg2EE lv2tQMbevTU4CY1yeEjdnBjO2NMEkj1J36ATmmAEEBG5Rt70yUp13bUs9GMl3kq8qsqq T4LuGccHOwZztRz9MdB2VpSRqKIlE9xZggmiu6d9h1EQ2f+iRcypHd1lD2Wigw9BxmHr NGSwnaIZSyH2Fe1X33ZYK8hYyVnA08y3NouiRTVZ1gh+dTA0Q/HcWXDmhwg9397GovGr Sp1+ARLkE0mUPQ477oTMeNs7YJQBwy5W/NEkJw99I13P370E/SEuozp50LsI/TmJ/Uav sX1A== X-Gm-Message-State: AOAM531bHpqf7H+7Ws1MgtuFJBPAatorcUp4liE7w+CI035g2K/0if2m 7DV7njGjKq5zSJd/1BmOUYHI4ibPKDRy9veF X-Google-Smtp-Source: ABdhPJzN9qq+RpE4nJQZQlwN3q2utoa7WEDbg5nPqkqJ5iIx5r1kNhb1ZiPOeysEe1i6wZZj/rSAVg== X-Received: by 2002:a17:902:b402:b029:d2:686a:4f43 with SMTP id x2-20020a170902b402b02900d2686a4f43mr2695724plr.34.1601137841446; Sat, 26 Sep 2020 09:30:41 -0700 (PDT) Received: from localhost (76-210-143-223.lightspeed.sntcca.sbcglobal.net. [76.210.143.223]) by smtp.gmail.com with ESMTPSA id e207sm6603365pfh.171.2020.09.26.09.30.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 26 Sep 2020 09:30:40 -0700 (PDT) Date: Sat, 26 Sep 2020 09:30:40 -0700 (PDT) X-Google-Original-Date: Sat, 26 Sep 2020 09:30:39 PDT (-0700) Subject: Re: PR26569, R_RISCV_RVC_JUMP results in buffer overflow In-Reply-To: <20200921001526.GW5452@bubble.grove.modra.org> CC: binutils@sourceware.org, Andrew Waterman , Jim Wilson From: Palmer Dabbelt To: amodra@gmail.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 26 Sep 2020 16:30:45 -0000 On Sun, 20 Sep 2020 17:15:26 PDT (-0700), amodra@gmail.com wrote: > On Sat, Sep 19, 2020 at 01:53:39PM -0700, Palmer Dabbelt wrote: >> On Thu, 17 Sep 2020 01:20:18 PDT (-0700), amodra@gmail.com wrote: >> > HOWTO (R_RISCV_ALIGN, /* type */ >> > 0, /* rightshift */ >> > - 2, /* size */ >> > + 3, /* size */ >> >> Can't these be up to a page in size? > > These relocs are special, and can't be handled by the howto struct and > generic code without implementing a special_function. So what is in > the howto doesn't matter very much but this "size" agrees with > "bitsize", "3" being the value to say the number of bytes is zero. > >> > 0, /* bitsize */ >> > FALSE, /* pc_relative */ >> > 0, /* bitpos */ >> > --- a/gas/config/tc-riscv.h >> > +++ b/gas/config/tc-riscv.h >> > @@ -77,6 +77,14 @@ extern int riscv_parse_long_option (const char *); >> > #define md_pre_output_hook riscv_pre_output_hook() >> > extern void riscv_pre_output_hook (void); >> > >> > +/* Call macro frags are tied off wrongly at the auipc which has an >> > + eight byte R_RISCV_CALL reloc covering both the auipc and >> > + immediately following jalr. For now, work around this by allowing >> > + CALL relocs to extend past the end of a frag. */ >> > +#define TC_FX_SIZE_SLACK(FIX) \ >> > + (((FIX)->fx_r_type == BFD_RELOC_RISCV_CALL \ >> > + || (FIX)->fx_r_type == BFD_RELOC_RISCV_CALL_PLT) ? 4 : 0) >> > + >> >> I haven't tried it yet, but I think something like this would eliminate the >> need for this slack? > > Almost. There needs to be a frag_grow(8) before auipc is output. > Otherwise if you're unlucky the frag runs out of room after the auipc > and you hit the same "fixup not contained within frag". > >> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c >> index 9df6d3f415..cec235b986 100644 >> --- a/gas/config/tc-riscv.c >> +++ b/gas/config/tc-riscv.c >> @@ -1133,9 +1133,7 @@ append_insn (struct riscv_cl_insn *ip, expressionS *address_expr, >> optimized away or compressed by the linker during relaxation, to prevent >> the assembler from computing static offsets across such an instruction. >> This is necessary to get correct EH info. */ >> - if (reloc_type == BFD_RELOC_RISCV_CALL >> - || reloc_type == BFD_RELOC_RISCV_CALL_PLT >> - || reloc_type == BFD_RELOC_RISCV_HI20 >> + if (reloc_type == BFD_RELOC_RISCV_HI20 >> || reloc_type == BFD_RELOC_RISCV_PCREL_HI20 >> || reloc_type == BFD_RELOC_RISCV_TPREL_HI20 >> || reloc_type == BFD_RELOC_RISCV_TPREL_ADD) >> @@ -1313,6 +1311,8 @@ riscv_call (int destreg, int tempreg, expressionS *ep, >> { >> macro_build (ep, "auipc", "d,u", tempreg, reloc); >> macro_build (NULL, "jalr", "d,s", destreg, tempreg); >> + frag_wane(frag_now); >> + frag_new(0); >> } >> >> /* Load an integer constant into a register. */ > > I've committed the following. Thanks! > ---- > > This patch corrects "size" and "bitsize" in R_RISCV_RVC_* reloc howtos > so that elfnn-riscv.c:perform_relocation doesn't access past the end > of a section. I've also corrected "size" in the R_RISCV_CALL* reloc > howtos since these relocs apply to two consecutive instructions. That > caused fallout in the assembler with complaints about "fixup not > contained within frag" due to tc-riscv.c:append_insn finishing off a > frag after the auipc insn making up a "call" macro. Which is a little > rude since the CALL reloc also relocates the following jalr. Fixed by > changing the frag handling a little. > > I've also changed R_RISCV_ALIGN and R_RISCV_TPREL_ADD marker reloc > howtos to look like R_RISCV_NONE, and corrected dst_mask for numerous > relocs, not that it matters very much. > > bfd/ > PR 26569 > * elfxx-riscv.c (howto_table): Correct size and bitsize of > R_RISCV_RVC_BRANCH, R_RISCV_RVC_JUMP, and R_RISCV_RVC_LUI. > Correct size for R_RISCV_TLS_DTPMOD32, R_RISCV_TLS_DTPREL32, > R_RISCV_CALL, and R_RISCV_CALL_PLT. Make R_RISCV_TPREL_ADD and > R_RISCV_ALIGN like R_RISCV_NONE. Correct dst_mask many relocs. > gas/ > * config/tc-riscv.c (append_insn): Don't tie off frags at CALL > relocs. > (riscv_call): Tie them off after the jalr. > (md_apply_fix): Zero fx_size of RELAX fixup. > > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c > index cfdd867e0d..e5adea57b1 100644 > --- a/bfd/elfxx-riscv.c > +++ b/bfd/elfxx-riscv.c > @@ -69,7 +69,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 64 bit relocation. */ > @@ -99,7 +99,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_RELATIVE", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > HOWTO (R_RISCV_COPY, /* type */ > @@ -133,7 +133,7 @@ static reloc_howto_type howto_table[] = > /* Dynamic TLS relocations. */ > HOWTO (R_RISCV_TLS_DTPMOD32, /* type */ > 0, /* rightshift */ > - 4, /* size */ > + 2, /* size */ > 32, /* bitsize */ > FALSE, /* pc_relative */ > 0, /* bitpos */ > @@ -142,7 +142,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_TLS_DTPMOD32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > HOWTO (R_RISCV_TLS_DTPMOD64, /* type */ > @@ -161,7 +161,7 @@ static reloc_howto_type howto_table[] = > > HOWTO (R_RISCV_TLS_DTPREL32, /* type */ > 0, /* rightshift */ > - 4, /* size */ > + 2, /* size */ > 32, /* bitsize */ > FALSE, /* pc_relative */ > 0, /* bitpos */ > @@ -170,7 +170,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_TLS_DTPREL32", /* name */ > TRUE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > HOWTO (R_RISCV_TLS_DTPREL64, /* type */ > @@ -198,7 +198,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_TLS_TPREL32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > HOWTO (R_RISCV_TLS_TPREL64, /* type */ > @@ -254,7 +254,7 @@ static reloc_howto_type howto_table[] = > /* 32-bit PC-relative function call (AUIPC/JALR). */ > HOWTO (R_RISCV_CALL, /* type */ > 0, /* rightshift */ > - 2, /* size */ > + 4, /* size */ > 64, /* bitsize */ > TRUE, /* pc_relative */ > 0, /* bitpos */ > @@ -270,7 +270,7 @@ static reloc_howto_type howto_table[] = > /* Like R_RISCV_CALL, but not locally binding. */ > HOWTO (R_RISCV_CALL_PLT, /* type */ > 0, /* rightshift */ > - 2, /* size */ > + 4, /* size */ > 64, /* bitsize */ > TRUE, /* pc_relative */ > 0, /* bitpos */ > @@ -466,14 +466,14 @@ static reloc_howto_type howto_table[] = > /* TLS LE thread pointer usage. May be relaxed. */ > HOWTO (R_RISCV_TPREL_ADD, /* type */ > 0, /* rightshift */ > - 2, /* size */ > - 32, /* bitsize */ > + 3, /* size */ > + 0, /* bitsize */ > FALSE, /* pc_relative */ > 0, /* bitpos */ > complain_overflow_dont, /* complain_on_overflow */ > bfd_elf_generic_reloc, /* special_function */ > "R_RISCV_TPREL_ADD", /* name */ > - TRUE, /* partial_inplace */ > + FALSE, /* partial_inplace */ > 0, /* src_mask */ > 0, /* dst_mask */ > FALSE), /* pcrel_offset */ > @@ -490,7 +490,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_ADD8", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 16-bit in-place addition, for local label subtraction. */ > @@ -505,7 +505,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_ADD16", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 32-bit in-place addition, for local label subtraction. */ > @@ -520,7 +520,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_ADD32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 64-bit in-place addition, for local label subtraction. */ > @@ -550,7 +550,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SUB8", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 16-bit in-place addition, for local label subtraction. */ > @@ -565,7 +565,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SUB16", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 32-bit in-place addition, for local label subtraction. */ > @@ -580,7 +580,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SUB32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 64-bit in-place addition, for local label subtraction. */ > @@ -633,7 +633,7 @@ static reloc_howto_type howto_table[] = > addend rounded up to the next power of two. */ > HOWTO (R_RISCV_ALIGN, /* type */ > 0, /* rightshift */ > - 2, /* size */ > + 3, /* size */ > 0, /* bitsize */ > FALSE, /* pc_relative */ > 0, /* bitpos */ > @@ -643,13 +643,13 @@ static reloc_howto_type howto_table[] = > FALSE, /* partial_inplace */ > 0, /* src_mask */ > 0, /* dst_mask */ > - TRUE), /* pcrel_offset */ > + FALSE), /* pcrel_offset */ > > /* 8-bit PC-relative branch offset. */ > HOWTO (R_RISCV_RVC_BRANCH, /* type */ > 0, /* rightshift */ > - 2, /* size */ > - 32, /* bitsize */ > + 1, /* size */ > + 16, /* bitsize */ > TRUE, /* pc_relative */ > 0, /* bitpos */ > complain_overflow_signed, /* complain_on_overflow */ > @@ -663,8 +663,8 @@ static reloc_howto_type howto_table[] = > /* 11-bit PC-relative jump offset. */ > HOWTO (R_RISCV_RVC_JUMP, /* type */ > 0, /* rightshift */ > - 2, /* size */ > - 32, /* bitsize */ > + 1, /* size */ > + 16, /* bitsize */ > TRUE, /* pc_relative */ > 0, /* bitpos */ > complain_overflow_dont, /* complain_on_overflow */ > @@ -678,8 +678,8 @@ static reloc_howto_type howto_table[] = > /* High 6 bits of 18-bit absolute address. */ > HOWTO (R_RISCV_RVC_LUI, /* type */ > 0, /* rightshift */ > - 2, /* size */ > - 32, /* bitsize */ > + 1, /* size */ > + 16, /* bitsize */ > FALSE, /* pc_relative */ > 0, /* bitpos */ > complain_overflow_dont, /* complain_on_overflow */ > @@ -807,7 +807,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SET8", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 16-bit in-place setting, for local label subtraction. */ > @@ -822,7 +822,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SET16", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 32-bit in-place setting, for local label subtraction. */ > @@ -837,7 +837,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_SET32", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > > /* 32-bit PC relative. */ > @@ -852,7 +852,7 @@ static reloc_howto_type howto_table[] = > "R_RISCV_32_PCREL", /* name */ > FALSE, /* partial_inplace */ > 0, /* src_mask */ > - MINUS_ONE, /* dst_mask */ > + 0xffffffff, /* dst_mask */ > FALSE), /* pcrel_offset */ > }; > > diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c > index 9df6d3f415..eb31e42a2e 100644 > --- a/gas/config/tc-riscv.c > +++ b/gas/config/tc-riscv.c > @@ -1133,9 +1133,7 @@ append_insn (struct riscv_cl_insn *ip, expressionS *address_expr, > optimized away or compressed by the linker during relaxation, to prevent > the assembler from computing static offsets across such an instruction. > This is necessary to get correct EH info. */ > - if (reloc_type == BFD_RELOC_RISCV_CALL > - || reloc_type == BFD_RELOC_RISCV_CALL_PLT > - || reloc_type == BFD_RELOC_RISCV_HI20 > + if (reloc_type == BFD_RELOC_RISCV_HI20 > || reloc_type == BFD_RELOC_RISCV_PCREL_HI20 > || reloc_type == BFD_RELOC_RISCV_TPREL_HI20 > || reloc_type == BFD_RELOC_RISCV_TPREL_ADD) > @@ -1311,8 +1309,13 @@ static void > riscv_call (int destreg, int tempreg, expressionS *ep, > bfd_reloc_code_real_type reloc) > { > + /* Ensure the jalr is emitted to the same frag as the auipc. */ > + frag_grow (8); > macro_build (ep, "auipc", "d,u", tempreg, reloc); > macro_build (NULL, "jalr", "d,s", destreg, tempreg); > + /* See comment at end of append_insn. */ > + frag_wane (frag_now); > + frag_new (0); > } > > /* Load an integer constant into a register. */ > @@ -3031,6 +3034,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED) > fixP->fx_next = xmemdup (fixP, sizeof (*fixP), sizeof (*fixP)); > fixP->fx_next->fx_addsy = fixP->fx_next->fx_subsy = NULL; > fixP->fx_next->fx_r_type = BFD_RELOC_RISCV_RELAX; > + fixP->fx_next->fx_size = 0; > } > }