From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id 85FA8385C6EB for ; Fri, 30 Jun 2023 08:50:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 85FA8385C6EB Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="6.01,170,1684828800"; d="scan'208,223";a="10367915" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 30 Jun 2023 00:50:31 -0800 IronPort-SDR: FjgTcIscjVQUi6LgQ5GyqJVdNu809VDIBurHGvlpcdnAcUKIcQ1dX7NJSrdQh6ZTzwUDYSU16t nmC028muipqGpPZ/+223a36I5qb6DWhZVTX6U16oLMMT9vX5C/O3SG/wN1GsqGTKGTfJn06cTE sZIlm0SbEIiy7Rcf4i7SjotraW8GvUG7uJfm/5USjlibsVCskmBznuF6769sM0RJMiRZHUPIc5 3TIloez4iguo1lCcozM4l5BEp4+TqsV7zFzAM+zVJ4Sjv694/mfh2lQMvBWVohwwkHeHJ+seho MnE= From: Thomas Schwinge To: Pan Li , , , Richard Biener , Jakub Jelinek CC: Robin Dapp , , , , Tobias Burnus Subject: [v4] Streamer: Fix out of range memory access of machine mode In-Reply-To: References: <20230619080710.1536456-1-pan2.li@intel.com> <20230621075824.1990571-1-pan2.li@intel.com> <874jmqwr8q.fsf@euler.schwinge.homeip.net> <87sfaauit8.fsf@dem-tschwing-1.ger.mentorg.com> <24F5CA0EC7859D5E+2023063009263644698181@rivai.ai> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/28.2 (x86_64-pc-linux-gnu) Date: Fri, 30 Jun 2023 10:50:17 +0200 Message-ID: <87mt0hcp12.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-15.mgc.mentorg.com (139.181.222.15) To svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! On 2023-06-30T01:39:39+0000, "Li, Pan2" wrote: > That=E2=80=99s very cool, thanks Thomas for help! :-) > Let=E2=80=99s wait the AMD test running result for the final version of t= he patch. That's all looking good, too. > From: juzhe.zhong@rivai.ai > Sent: Friday, June 30, 2023 9:27 AM > Could you merge your patch after you tested? I've done that, and with (already approved) "LTO: Capture 'lto_file_decl_data *file_data' in 'class lto_input_block'" split out, OK to push the attached v4 "Streamer: Fix out of range memory access of machine mode"? Gr=C3=BC=C3=9Fe Thomas > From: Thomas Schwinge > Date: 2023-06-30 04:14 > Subject: Re: [PATCH v3] Streamer: Fix out of range memory access of machi= ne mode > Hi! > > On 2023-06-29T11:29:57+0200, I wrote: >> On 2023-06-21T15:58:24+0800, Pan Li via Gcc-patches > wrote: >>> We extend the machine mode from 8 to 16 bits already. But there still >>> one placing missing from the streamer. It has one hard coded array >>> for the machine code like size 256. >>> >>> In the lto pass, we memset the array by MAX_MACHINE_MODE count but the >>> value of the MAX_MACHINE_MODE will grow as more and more modes are >>> added. While the machine mode array in tree-streamer still leave 256 as= is. >>> >>> Then, when the MAX_MACHINE_MODE is greater than 256, the memset of >>> lto_output_init_mode_table will touch the memory out of range unexpecte= d. >> >> Uh. :-O >> >>> This patch would like to take the MAX_MACHINE_MODE as the size of the >>> array in streamer, to make sure there is no potential unexpected >>> memory access in future. Meanwhile, this patch also adjust some place >>> which has MAX_MACHINE_MODE <=3D 256 assumption. >> >> Thanks to Jakub and Richard for guidance re the offloading compilation >> case, where we've got different 'MAX_MACHINE_MODE's between stream-out >> and stream-in, and a modes mapping table. >> >> However, with this patch, there are ICEs all over the place... I'm >> having a look. > > Your patch has all the right ideas, there are just a few additional > changes necessary. Please merge in the attached > "f into Streamer: Fix out of range memory access of machine mode", with > 'Co-authored-by: Thomas Schwinge >'. This has > already survived compiler-side 'lto.exp' testing and > 'check-target-libgomp' with Nvidia GPU offloading; AMD GPU testing is now > running (not expecting any bad surprises). Will let you know by (my) > tomorrow morning in case there are any more problems. > > Explanation: > >>> --- a/gcc/lto-streamer-in.cc >>> +++ b/gcc/lto-streamer-in.cc >>> @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *= file_data) >>> internal_error ("cannot read LTO mode table from %s", >>> file_data->file_name); >>> >>> - unsigned char *table =3D ggc_cleared_vec_alloc (1 << = 8); >>> - file_data->mode_table =3D table; >>> const struct lto_simple_header_with_strings *header >>> =3D (const struct lto_simple_header_with_strings *) data; >>> int string_offset; >>> @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data= *file_data) >>> header->string_size, vNULL); >>> bitpack_d bp =3D streamer_read_bitpack (&ib); >>> >>> + unsigned mode_bits =3D bp_unpack_value (&bp, 5); >>> + unsigned char *table =3D ggc_cleared_vec_alloc (1 << = mode_bits); >>> + >>> + file_data->mode_table =3D table; >>> + file_data->mode_bits =3D mode_bits; > > Here, we set 'file_data->mode_bits' for the offloading case (where > 'lto_input_mode_table' is called) -- but it's not set for the > non-offloading case (where 'lto_input_mode_table' isn't called). (See my > 'gcc/lto/lto-common.cc:lto_read_decls' change.) That's "not currently a > problem", as 'file_data->mode_bits' isn't used anywhere... > >>> --- a/gcc/lto-streamer.h >>> +++ b/gcc/lto-streamer.h >>> @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data >>> int order_base; >>> >>> int unit_base; >>> + >>> + unsigned mode_bits; >>> }; > >>> inline machine_mode >>> bp_unpack_machine_mode (struct bitpack_d *bp) >>> { >>> - return (machine_mode) >>> - ((class lto_input_block *) >>> - bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 <= < 8)]; >>> + int last =3D 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + lto_input_block *input_block =3D (class lto_input_block *) bp->strea= m; >>> + int index =3D bp_unpack_enum (bp, machine_mode, last); >>> + >>> + return (machine_mode) input_block->mode_table[index]; >>> } > > ..., but 'file_data->mode_bits' needs to be considered here, in the > stream-in for offloading, where 'file_data->mode_bits' -- that is, the > host 'MAX_MACHINE_MODE' -- very likely is different from the offload > device 'MAX_MACHINE_MODE'. > > Easiest is in 'gcc/lto-streamer.h:class lto_input_block' to capture > 'lto_file_decl_data *file_data' instead of just > 'unsigned char *mode_table', and adjust all users. > > That's it. :-) > >>> --- a/gcc/tree-streamer.h >>> +++ b/gcc/tree-streamer.h > >>> @@ -108,15 +108,19 @@ inline void >>> bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) >>> { >>> streamer_mode_table[mode] =3D 1; >>> - bp_pack_enum (bp, machine_mode, 1 << 8, mode); >>> + int last =3D 1 << ceil_log2 (MAX_MACHINE_MODE); >>> + >>> + bp_pack_enum (bp, machine_mode, last, mode); >>> } > > That use of 'MAX_MACHINE_MODE' is safe, as that only concerns the > stream-out phase. > >>> --- a/gcc/tree-streamer.cc >>> +++ b/gcc/tree-streamer.cc >>> @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see >>> During streaming in, we translate the on the disk mode using this >>> table. For normal LTO it is set to identity, for ACCEL_COMPILER >>> depending on the mode_table content. */ >>> -unsigned char streamer_mode_table[1 << 8]; >>> +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; > > Likewise. > > > Gr=C3=BC=C3=9Fe > Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-Streamer-Fix-out-of-range-memory-access-of-machine-m.patch" >From f61d1ffbf0d1e1aa65dcb96e9b32b4e2fca7ed69 Mon Sep 17 00:00:00 2001 From: Pan Li Date: Wed, 21 Jun 2023 15:58:24 +0800 Subject: [PATCH] Streamer: Fix out of range memory access of machine mode We extend the machine mode from 8 to 16 bits already. But there still one placing missing from the streamer. It has one hard coded array for the machine code like size 256. In the lto pass, we memset the array by MAX_MACHINE_MODE count but the value of the MAX_MACHINE_MODE will grow as more and more modes are added. While the machine mode array in tree-streamer still leave 256 as is. Then, when the MAX_MACHINE_MODE is greater than 256, the memset of lto_output_init_mode_table will touch the memory out of range unexpected. This patch would like to take the MAX_MACHINE_MODE as the size of the array in streamer, to make sure there is no potential unexpected memory access in future. Meanwhile, this patch also adjust some place which has MAX_MACHINE_MODE <= 256 assumption. Care is taken that for offload compilation, we interpret the stream-in data in terms of the host 'MAX_MACHINE_MODE' ('file_data->mode_bits'), which very likely is different from the offload device 'MAX_MACHINE_MODE'. gcc/ * lto-streamer-in.cc (lto_input_mode_table): Stream in the mode bits for machine mode table. * lto-streamer-out.cc (lto_write_mode_table): Stream out the HOST machine mode bits. * lto-streamer.h (struct lto_file_decl_data): New fields mode_bits. * tree-streamer.cc (streamer_mode_table): Take MAX_MACHINE_MODE as the table size. * tree-streamer.h (streamer_mode_table): Ditto. (bp_pack_machine_mode): Take 1 << ceil_log2 (MAX_MACHINE_MODE) as the packing limit. (bp_unpack_machine_mode): Ditto with 'file_data->mode_bits'. gcc/lto/ * lto-common.cc (lto_file_finalize) [!ACCEL_COMPILER]: Initialize 'file_data->mode_bits'. Signed-off-by: Pan Li Co-authored-by: Thomas Schwinge --- gcc/lto-streamer-in.cc | 12 ++++++++---- gcc/lto-streamer-out.cc | 11 ++++++++--- gcc/lto-streamer.h | 2 ++ gcc/lto/lto-common.cc | 1 + gcc/tree-streamer.cc | 2 +- gcc/tree-streamer.h | 8 +++++--- 6 files changed, 25 insertions(+), 11 deletions(-) diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc index 6e8bc9516a6..1876e1967ec 100644 --- a/gcc/lto-streamer-in.cc +++ b/gcc/lto-streamer-in.cc @@ -1985,8 +1985,6 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) internal_error ("cannot read LTO mode table from %s", file_data->file_name); - unsigned char *table = ggc_cleared_vec_alloc (1 << 8); - file_data->mode_table = table; const struct lto_simple_header_with_strings *header = (const struct lto_simple_header_with_strings *) data; int string_offset; @@ -1998,16 +1996,22 @@ lto_input_mode_table (struct lto_file_decl_data *file_data) header->string_size, vNULL); bitpack_d bp = streamer_read_bitpack (&ib); + unsigned mode_bits = bp_unpack_value (&bp, 5); + unsigned char *table = ggc_cleared_vec_alloc (1 << mode_bits); + + file_data->mode_table = table; + file_data->mode_bits = mode_bits; + table[VOIDmode] = VOIDmode; table[BLKmode] = BLKmode; unsigned int m; - while ((m = bp_unpack_value (&bp, 8)) != VOIDmode) + while ((m = bp_unpack_value (&bp, mode_bits)) != VOIDmode) { enum mode_class mclass = bp_unpack_enum (&bp, mode_class, MAX_MODE_CLASS); poly_uint16 size = bp_unpack_poly_value (&bp, 16); poly_uint16 prec = bp_unpack_poly_value (&bp, 16); - machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8); + machine_mode inner = (machine_mode) bp_unpack_value (&bp, mode_bits); poly_uint16 nunits = bp_unpack_poly_value (&bp, 16); unsigned int ibit = 0, fbit = 0; unsigned int real_fmt_len = 0; diff --git a/gcc/lto-streamer-out.cc b/gcc/lto-streamer-out.cc index 5ab2eb4301e..36899283ded 100644 --- a/gcc/lto-streamer-out.cc +++ b/gcc/lto-streamer-out.cc @@ -3196,6 +3196,11 @@ lto_write_mode_table (void) if (inner_m != m) streamer_mode_table[(int) inner_m] = 1; } + + /* Pack the mode_bits value within 5 bits (up to 31) in the beginning. */ + unsigned mode_bits = ceil_log2 (MAX_MACHINE_MODE); + bp_pack_value (&bp, mode_bits, 5); + /* First stream modes that have GET_MODE_INNER (m) == m, so that we can refer to them afterwards. */ for (int pass = 0; pass < 2; pass++) @@ -3205,11 +3210,11 @@ lto_write_mode_table (void) machine_mode m = (machine_mode) i; if ((GET_MODE_INNER (m) == m) ^ (pass == 0)) continue; - bp_pack_value (&bp, m, 8); + bp_pack_value (&bp, m, mode_bits); bp_pack_enum (&bp, mode_class, MAX_MODE_CLASS, GET_MODE_CLASS (m)); bp_pack_poly_value (&bp, GET_MODE_SIZE (m), 16); bp_pack_poly_value (&bp, GET_MODE_PRECISION (m), 16); - bp_pack_value (&bp, GET_MODE_INNER (m), 8); + bp_pack_value (&bp, GET_MODE_INNER (m), mode_bits); bp_pack_poly_value (&bp, GET_MODE_NUNITS (m), 16); switch (GET_MODE_CLASS (m)) { @@ -3229,7 +3234,7 @@ lto_write_mode_table (void) } bp_pack_string (ob, &bp, GET_MODE_NAME (m), true); } - bp_pack_value (&bp, VOIDmode, 8); + bp_pack_value (&bp, VOIDmode, mode_bits); streamer_write_bitpack (&bp); diff --git a/gcc/lto-streamer.h b/gcc/lto-streamer.h index 2913b808bde..0556b34c837 100644 --- a/gcc/lto-streamer.h +++ b/gcc/lto-streamer.h @@ -604,6 +604,8 @@ struct GTY(()) lto_file_decl_data int order_base; int unit_base; + + unsigned mode_bits; }; typedef struct lto_file_decl_data *lto_file_decl_data_ptr; diff --git a/gcc/lto/lto-common.cc b/gcc/lto/lto-common.cc index c235df8a5f5..973ab791712 100644 --- a/gcc/lto/lto-common.cc +++ b/gcc/lto/lto-common.cc @@ -2275,6 +2275,7 @@ lto_file_finalize (struct lto_file_decl_data *file_data, lto_file *file, lto_input_mode_table (file_data); #else file_data->mode_table = lto_mode_identity_table; + file_data->mode_bits = ceil_log2 (MAX_MACHINE_MODE); #endif data = lto_get_summary_section_data (file_data, LTO_section_decls, &len); diff --git a/gcc/tree-streamer.cc b/gcc/tree-streamer.cc index ed65a7692e3..a28ef9c7920 100644 --- a/gcc/tree-streamer.cc +++ b/gcc/tree-streamer.cc @@ -35,7 +35,7 @@ along with GCC; see the file COPYING3. If not see During streaming in, we translate the on the disk mode using this table. For normal LTO it is set to identity, for ACCEL_COMPILER depending on the mode_table content. */ -unsigned char streamer_mode_table[1 << 8]; +unsigned char streamer_mode_table[MAX_MACHINE_MODE]; /* Check that all the TS_* structures handled by the streamer_write_* and streamer_read_* routines are exactly ALL the structures defined in diff --git a/gcc/tree-streamer.h b/gcc/tree-streamer.h index 6d565acbbc2..ff49d1ba637 100644 --- a/gcc/tree-streamer.h +++ b/gcc/tree-streamer.h @@ -75,7 +75,7 @@ void streamer_write_tree_body (struct output_block *, tree); void streamer_write_integer_cst (struct output_block *, tree); /* In tree-streamer.cc. */ -extern unsigned char streamer_mode_table[1 << 8]; +extern unsigned char streamer_mode_table[MAX_MACHINE_MODE]; void streamer_check_handled_ts_structures (void); bool streamer_tree_cache_insert (struct streamer_tree_cache_d *, tree, hashval_t, unsigned *); @@ -108,14 +108,16 @@ inline void bp_pack_machine_mode (struct bitpack_d *bp, machine_mode mode) { streamer_mode_table[mode] = 1; - bp_pack_enum (bp, machine_mode, 1 << 8, mode); + int last = 1 << ceil_log2 (MAX_MACHINE_MODE); + bp_pack_enum (bp, machine_mode, last, mode); } inline machine_mode bp_unpack_machine_mode (struct bitpack_d *bp) { lto_input_block *ib = (class lto_input_block *) bp->stream; - unsigned ix = bp_unpack_enum (bp, machine_mode, 1 << 8); + int last = 1 << ib->file_data->mode_bits; + unsigned ix = bp_unpack_enum (bp, machine_mode, last); return (machine_mode) ib->file_data->mode_table[ix]; } -- 2.34.1 --=-=-=--