From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd36.google.com (mail-io1-xd36.google.com [IPv6:2607:f8b0:4864:20::d36]) by sourceware.org (Postfix) with ESMTPS id 38FA63857C49 for ; Thu, 17 Sep 2020 10:10:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 38FA63857C49 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sifive.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nelson.chu@sifive.com Received: by mail-io1-xd36.google.com with SMTP id d190so1569875iof.3 for ; Thu, 17 Sep 2020 03:10:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=k5NkbUbRf8s0nUOsNOy/dE/0gyXNx34OvUk7I05xriQ=; b=g/Lo3pTlripy2sNRjVLf0ko/mhj+ITjBmNH2FozW/Rnip4I0p36kZH1YiUb5lO/c7z GBruWcz/qn/fwhyqI1Pddy7clMw/ipRZw6iQZV4qZ0bUcIGTcEXy1GDPjeh8NeobfT76 9q1CxOIN4qfPGI721C7MTsjVC5z34jpsz135XtmWiBUzApPUBgqFtRF8RgcFVDPJInrO 1YnoY1FflzVzptCu8ICKJLtbuJM/RGHX0Vze7fUMNFT9VCudz3+3mvST79F3pBv09Ze8 1+7uhSP02aRbFIWzBVmu1DC203EZCP8DMxA+40A4MG1l5DfeBdg5dofC+IIz6/74nk/b m6sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=k5NkbUbRf8s0nUOsNOy/dE/0gyXNx34OvUk7I05xriQ=; b=pYxjbawPYgwBgOaBi3KDMzywwa35IXbK4/LO6uS7IpIdP+qB8z+NT5Ih7AysQPYpKy oc/f+qKBMl58nq468Ytu8qlklW2Tz/A1jApwaMTF4kIBCQG3nu21mi6K9F9h4mID3cGV UxhXe82dCD7HREfwLWwOrYJigSX4xPeE0QO3g1b+KCZGmYCIDl8zHl7P/07SSdTeTc74 zW3gdxyUcxs2OBS+m/hqbxhxtj+GVBolyjKG/eFv5sZsuTtKD13YF3sG1mflcLWvXuR0 mYEXQgJBjHA2iEKIadSSueHbqbUSAc8G16Nq7WvsMxgTrPTFFrNk5VosRjtEDOUzNFft nMQg== X-Gm-Message-State: AOAM531/lztzwVPa7m8Sh1iaOv3zryI4CpUBImbuBOpmRLx4smvwlbk0 VC+1Yrtbg1PrcT9Hek21Tb4agHhg3re0S93x2BtNNA== X-Google-Smtp-Source: ABdhPJy6ltYmmO8uA6Xc8LaiSHmeA1AukkNM+Wp/yBKotrA8cxjP2/+4VF3KX/hYqjucxM8Yc0fD7kKEQ3Lh/KKpjao= X-Received: by 2002:a6b:8dcc:: with SMTP id p195mr22376823iod.39.1600337423458; Thu, 17 Sep 2020 03:10:23 -0700 (PDT) MIME-Version: 1.0 References: <20200917082018.GR5452@bubble.grove.modra.org> In-Reply-To: <20200917082018.GR5452@bubble.grove.modra.org> From: Nelson Chu Date: Thu, 17 Sep 2020 18:10:12 +0800 Message-ID: Subject: Re: PR26569, R_RISCV_RVC_JUMP results in buffer overflow To: Alan Modra Cc: Binutils Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, 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: Thu, 17 Sep 2020 10:10:26 -0000 Hi Alan, LGTM, thanks for fixing them. I have quickly tested my commonly used toolchain (rv32i/rv64gc), and everything looks good for now. But I'm not a riscv FSF binutils maintainer, so I can not approve anything. Just to mention that we may also encounter the similar size problems in the future, and thank you for fixing it now. Nelson On Thu, Sep 17, 2020 at 4:20 PM Alan Modra via Binutils wrote: > > 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. I wasn't > game to change the frag handling, so worked around the error using > TC_FIX_SIZE_SLACK, and adjusted fx_size for the RELAX reloc following > a CALL. > > 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. > > OK to apply? > > 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 (md_apply_fix): Zero fx_size of RELAX fixup. > * config/tc-riscv.g (TC_FX_SIZE_SLACK): Define. > > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c > index cfdd867e0d..181969521a 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 */ > @@ -648,8 +648,8 @@ static reloc_howto_type howto_table[] = > /* 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..e1f7673eb5 100644 > --- a/gas/config/tc-riscv.c > +++ b/gas/config/tc-riscv.c > @@ -3031,6 +3031,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; > } > } > > diff --git a/gas/config/tc-riscv.h b/gas/config/tc-riscv.h > index 43d751ad0d..83cff8f2df 100644 > --- 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) > + > /* Let the linker resolve all the relocs due to relaxation. */ > #define tc_fix_adjustable(fixp) 0 > #define md_allow_local_subtract(l,r,s) 0 > > -- > Alan Modra > Australia Development Lab, IBM