RE: [PATCH] RISC-V: Fix out of range memory access of machine mode table

["Li, Pan2" <pan2.li@intel.com>]

Thanks Jakub for the useful comments, go thru the mail list and have a refinement version as below. But I not sure if I understand correct about adding new field named mode_bits in struct lto_file_decl_data, looks unnecessary up to a point.

Thanks again for your coaching with patient.

diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc
index 2cb83406db5..2a0720b4e6f 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<unsigned char> (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<unsigned char> (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..77250ee2385 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) of 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 fc7133d07ba..443f0cd616e 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/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 170d61cf20b..10718b03640 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,15 +108,19 @@ 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)
 {
-  return (machine_mode)
-	   ((class lto_input_block *)
-	    bp->stream)->mode_table[bp_unpack_enum (bp, machine_mode, 1 << 8)];
+  int last = 1 << ceil_log2 (MAX_MACHINE_MODE);
+  lto_input_block *input_block =  (class lto_input_block *) bp->stream;
+  int index = bp_unpack_enum (bp, machine_mode, last);
+
+  return (machine_mode) input_block->mode_table[index];
 }
 
 #endif  /* GCC_TREE_STREAMER_H  */

Pan

-----Original Message-----
From: Jakub Jelinek <jakub@redhat.com> 
Sent: Tuesday, June 20, 2023 11:26 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com; rguenther@suse.de
Subject: Re: [PATCH] RISC-V: Fix out of range memory access of machine mode table

On Tue, Jun 20, 2023 at 02:08:07PM +0000, Li, Pan2 via Gcc-patches wrote:
> Thanks Jakub for the explanation, I have a try like below patch but I am not quite sure it is expected, and where should I put the assertion.
> 
> > If yes, it needs to
> > be unsigned short, if not, we should add an assertion (e.g. on streaming
> > in the LTO table) that MAX_MACHINE_MODE <= 256.
> 
> diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc
> index 2cb83406db5..93ef97ec5d3 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<unsigned char> (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;
> @@ -1994,6 +1992,9 @@ lto_input_mode_table (struct lto_file_decl_data *file_data)
>    string_offset = sizeof (*header) + header->main_size;
>  
>    lto_input_block ib (data + sizeof (*header), header->main_size, NULL);
> +  unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (
> +    1 << ib.mode_bits);
> +  file_data->mode_table = table;
>    data_in = lto_data_in_create (file_data, data + string_offset,
>  				header->string_size, vNULL);
>    bitpack_d bp = streamer_read_bitpack (&ib);

Your ib.mode_bits is again the same ceil_log2 (MAX_MACHINE_MODE) value.
You need to stream that value out in lto-streamer-out.cc as perhaps the
first thing in the bitpack and stream it back here, so some
   mode_bits = bp_unpack_value (&bp, 5);
or so (perhaps 4 would be enough if we only support up to 15 bits for mode).
I.e. tell the offloading compiler what value had the host compiler when
streaming LTO out.

Then move those 3 lines from the above after that.  I'd put it next to
mode_table, so file_data->mode_bits.

The 
  unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (
    1 << ib.mode_bits);
formatting is wrong, ( shouldn't if at all possible be the last character
on a line.
In this case,
> --- a/gcc/lto-streamer.h
> +++ b/gcc/lto-streamer.h
> @@ -352,6 +352,8 @@ public:
>  
>    const char *data;
>    const unsigned char *mode_table;
> +  /* Indicates how many bits of one machine mode will have.  */
> +  const unsigned int mode_bits = ceil_log2 (MAX_MACHINE_MODE) ;

As I said earlier, I'd put it elsewhere.  The formatting is wrong
(no space before semicolon) and please don't add NSDMIs in structures
which don't have them already.

>  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)];
> +  lto_input_block *input_block =  (class lto_input_block *)bp->stream;

Wrong formatting again, there shouldn't be two consecutive spaces in there.  On the
other hand, there should be a space between *) and bp.

> +  int index = bp_unpack_enum (bp, machine_mode, input_block->mode_bits);
> +
> +  return (machine_mode)input_block->mode_table[index];

And here similarly.

	Jakub