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

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Jun 20, 2023 at 07:50:00AM +0000, Li, Pan2 wrote:
> Hi Jakub,
> 
> Thanks for reviewing but I am not quite sure if I fully understand how to fix this issue. Could you please help to enlighten me more about this ?
> 
> Currently for RISC-V, the memset has touched out of range memory already due to MAX_MACHINE_MODE > 256. And we may have below parts require adjusting.
> 
> 1. streamer_mode_table.
> 2.  bp_unpack_machine_mode/bp_pack_machine_mode 
> 3.  bp_pack_value/bp_unpack_value in lto_write_mode_table.
> 4. unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8) in lto_input_mode_table.
> 
> For 1. is safe to extend the size to MAX_MACHINE_MODE as the array only used as Boolean, aka streamer_mode_table[XXXmode] = 1.

Because the array is used only during stream out, that is safe.

> For 2 & 3. Keep 1 << 8 as is, or stream out the host MAX_MACHINE_MODE value somewhere for underlying consuming?

You can't keep 1 << 8, otherwise you won't stream all the bits.
I think you want to use 1 << ceil_log2 (MAX_MACHINE_MODE) on the stream out
side, stream that ceil_log2 (MAX_MACHINE_MODE) value somewhere at the start
of the mode table, add some field next to mode_table in lto_input_block
which will contain that value (and make sure to initialize it to
ceil_log2 (MAX_MACHINE_MODE) in case mode table isn't streamed in and use
1 << ...->mode_bits in e.g. bp_unpack_machine_mode
Or for cases where 8 was used before use ceil_log2 (MAX_MACHINE_MODE)
or mode_bits.

> For 4, one possible approach is that extend unsigned char to unsigned short, as well as 256 to MAX_MACHINE_MODE. Because it stores the actually machine mode in array.

The 1 << 8 needs to be similarly 1 << ...->mode_bits or ...->num_modes (that
is also streamed out and in), it is sized by the host number of modes.
Whether it is unsigned char or unsigned short array depends on if we
want to support offloading targets with > 256 modes.  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.

	Jakub