[v1] RISC-V: Fix out of range memory access when lto mode init
Checks
Commit Message
From: Pan Li <pan2.li@intel.com>
We extend the machine mode from 8 to 16 bits already. But there still
one placing missing from the tree-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 tree-streamer, to make sure there is no potential unexpected
memory access in future.
Signed-off-by: Pan Li <pan2.li@intel.com>
gcc/ChangeLog:
* tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE
as array size.
* tree-streamer.h (streamer_mode_table): Ditto.
---
gcc/tree-streamer.cc | 2 +-
gcc/tree-streamer.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
Comments
Add Richard Biener for reviewing, sorry for inconvenient.
Pan
-----Original Message-----
From: Li, Pan2 <pan2.li@intel.com>
Sent: Monday, June 19, 2023 4:07 PM
To: gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com
Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init
From: Pan Li <pan2.li@intel.com>
We extend the machine mode from 8 to 16 bits already. But there still
one placing missing from the tree-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 tree-streamer, to make sure there is no potential unexpected
memory access in future.
Signed-off-by: Pan Li <pan2.li@intel.com>
gcc/ChangeLog:
* tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE
as array size.
* tree-streamer.h (streamer_mode_table): Ditto.
---
gcc/tree-streamer.cc | 2 +-
gcc/tree-streamer.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
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..51a292c8d80 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 *);
On Mon, 19 Jun 2023, Li, Pan2 wrote:
> Add Richard Biener for reviewing, sorry for inconvenient.
>
> Pan
>
> -----Original Message-----
> From: Li, Pan2 <pan2.li@intel.com>
> Sent: Monday, June 19, 2023 4:07 PM
> To: gcc-patches@gcc.gnu.org
> Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com
> Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init
>
> From: Pan Li <pan2.li@intel.com>
>
> We extend the machine mode from 8 to 16 bits already. But there still
> one placing missing from the tree-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 tree-streamer, to make sure there is no potential unexpected
> memory access in future.
You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which
streams exactly values in [0, 1<<8 - 1].
CCing Jakub who invented this code.
Richard.
> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
> * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE
> as array size.
> * tree-streamer.h (streamer_mode_table): Ditto.
> ---
> gcc/tree-streamer.cc | 2 +-
> gcc/tree-streamer.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> 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..51a292c8d80 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 *);
>
Thanks Richard for the review, just go thru the word (1 << 8) and found another one besides bp. Update the PATCH v2 as below.
https://gcc.gnu.org/pipermail/gcc-patches/2023-June/622151.html
Pan
-----Original Message-----
From: Richard Biener <rguenther@suse.de>
Sent: Monday, June 19, 2023 4:41 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; Jakub Jelinek <jakub@redhat.com>
Subject: RE: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init
On Mon, 19 Jun 2023, Li, Pan2 wrote:
> Add Richard Biener for reviewing, sorry for inconvenient.
>
> Pan
>
> -----Original Message-----
> From: Li, Pan2 <pan2.li@intel.com>
> Sent: Monday, June 19, 2023 4:07 PM
> To: gcc-patches@gcc.gnu.org
> Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gmail.com; jeffreyalaw@gmail.com; Li, Pan2 <pan2.li@intel.com>; Wang, Yanzhang <yanzhang.wang@intel.com>; kito.cheng@gmail.com
> Subject: [PATCH v1] RISC-V: Fix out of range memory access when lto mode init
>
> From: Pan Li <pan2.li@intel.com>
>
> We extend the machine mode from 8 to 16 bits already. But there still
> one placing missing from the tree-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 tree-streamer, to make sure there is no potential unexpected
> memory access in future.
You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which
streams exactly values in [0, 1<<8 - 1].
CCing Jakub who invented this code.
Richard.
> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
> * tree-streamer.cc (streamer_mode_table): Use MAX_MACHINE_MODE
> as array size.
> * tree-streamer.h (streamer_mode_table): Ditto.
> ---
> gcc/tree-streamer.cc | 2 +-
> gcc/tree-streamer.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> 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..51a292c8d80 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 *);
>
On Mon, Jun 19, 2023 at 08:40:58AM +0000, Richard Biener wrote:
> You also have to fix bp_pack_machine_mode/bp_unpack_machine_mode which
> streams exactly values in [0, 1<<8 - 1].
>
> CCing Jakub who invented this code.
For stream-out, all it stores is a bool flag whether the mode is streamed
out, and on stream in it contains a mapping table between host and
offloading modes.
For stream in, it actually isn't used despite the comment maybe suggesting
it is, so I guess using MAX_MACHINE_MODE for it is ok.
As you said,
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);
}
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)];
}
needs changing for the case when MAX_MACHINE_MODE > 256, but far more
places make similar assumptions:
E.g. lto_write_mode_table has
bp_pack_value (&bp, m, 8);
(if MAX_MACHINE_MODE > 256, this can't encode all modes),
bp_pack_value (&bp, GET_MODE_INNER (m), 8);
(ditto).
lto_input_mode_table has e.g.
unsigned char *table = ggc_cleared_vec_alloc<unsigned char> (1 << 8);
file_data->mode_table = table;
Here we need to decide if we keep requiring that offloading architectures
still have MAX_MACHINE_MODE <= 256 or not. Currently the offloading
arches are nvptx and amdgcn, intelmic support has been removed.
If yes, table can have unsigned char elements, but its size actually depends
on the number of modes on the host side, so lto_write_mode_table would need
to stream out the host MAX_MACHINE_MODE value and lto_input_mode_table
stream it in and use instead of the 1 << 8 size above.
If not, mode_table and unsigned char * would need to change to unsigned
short *, or just conditionally depending on if MAX_MACHINE_MODE <= 256 or
not.
Then
while ((m = bp_unpack_value (&bp, 8)) != VOIDmode)
{
...
machine_mode inner = (machine_mode) bp_unpack_value (&bp, 8);
again hardcode 8 bits, that needs to match how many bits packs the host
compiler in lto_write_mode_table.
Jakub
@@ -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
@@ -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 *);