| Message ID | 20240709124757.1405749-4-christoph.muellner@vrull.eu |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Rewrite target arch attribute handling | expand |
IIRC Jeff mentions that it may introduce buffer overflow if the input string is long enough. On Tue, Jul 9, 2024 at 8:48 PM Christoph Müllner <christoph.muellner@vrull.eu> wrote: > > Allocating an object on the heap with new, wrapping it in a > std::unique_ptr and finally getting the buffer via buf.get() > is a correct way to allocate a buffer that is automatically > freed on return. However, a simple invocation of alloca() > does the same with less overhead. > > gcc/ChangeLog: > > * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::parse_arch): > Replace new + std::unique_ptr by alloca(). > (riscv_process_one_target_attr): Likewise. > (riscv_process_target_attr): Likewise. > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> > --- > gcc/config/riscv/riscv-target-attr.cc | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc > index 19eb7b06d548..e59cc53f23c6 100644 > --- a/gcc/config/riscv/riscv-target-attr.cc > +++ b/gcc/config/riscv/riscv-target-attr.cc > @@ -109,8 +109,7 @@ riscv_target_attr_parser::parse_arch (const char *str) > { > /* Parsing the extension list like "+<ext>[,+<ext>]*". */ > size_t len = strlen (str); > - std::unique_ptr<char[]> buf (new char[len+1]); > - char *str_to_check = buf.get (); > + char *str_to_check = (char *) alloca (len + 1); > strcpy (str_to_check, str); > const char *token = strtok_r (str_to_check, ",", &str_to_check); > m_subset_list = riscv_cmdline_subset_list ()->clone (); > @@ -247,8 +246,7 @@ riscv_process_one_target_attr (char *arg_str, > return false; > } > > - std::unique_ptr<char[]> buf (new char[len+1]); > - char *str_to_check = buf.get(); > + char *str_to_check = (char *) alloca (len + 1); > strcpy (str_to_check, arg_str); > > char *arg = strchr (str_to_check, '='); > @@ -334,8 +332,7 @@ riscv_process_target_attr (tree fndecl, tree args, location_t loc, > return false; > } > > - std::unique_ptr<char[]> buf (new char[len+1]); > - char *str_to_check = buf.get (); > + char *str_to_check = (char *) alloca (len + 1); > strcpy (str_to_check, TREE_STRING_POINTER (args)); > > /* Used to catch empty spaces between commas i.e. > -- > 2.45.2 >
On Tue, Jul 9, 2024 at 3:01 PM Kito Cheng <kito.cheng@sifive.com> wrote: > > IIRC Jeff mentions that it may introduce buffer overflow if the input > string is long enough. The manpage states: If the allocation causes stack overflow, program behavior is undefined. This was considered reasonable for AArch64: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/aarch64/aarch64.cc;h=7f0cc47d0f071de9297068baa85c6d5fc4d7fa5b;hb=HEAD#l19408 So, my assumption was that it should be fine for RISC-V as well. However, I won't insist on this patch. > > On Tue, Jul 9, 2024 at 8:48 PM Christoph Müllner > <christoph.muellner@vrull.eu> wrote: > > > > Allocating an object on the heap with new, wrapping it in a > > std::unique_ptr and finally getting the buffer via buf.get() > > is a correct way to allocate a buffer that is automatically > > freed on return. However, a simple invocation of alloca() > > does the same with less overhead. > > > > gcc/ChangeLog: > > > > * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::parse_arch): > > Replace new + std::unique_ptr by alloca(). > > (riscv_process_one_target_attr): Likewise. > > (riscv_process_target_attr): Likewise. > > > > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> > > --- > > gcc/config/riscv/riscv-target-attr.cc | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc > > index 19eb7b06d548..e59cc53f23c6 100644 > > --- a/gcc/config/riscv/riscv-target-attr.cc > > +++ b/gcc/config/riscv/riscv-target-attr.cc > > @@ -109,8 +109,7 @@ riscv_target_attr_parser::parse_arch (const char *str) > > { > > /* Parsing the extension list like "+<ext>[,+<ext>]*". */ > > size_t len = strlen (str); > > - std::unique_ptr<char[]> buf (new char[len+1]); > > - char *str_to_check = buf.get (); > > + char *str_to_check = (char *) alloca (len + 1); > > strcpy (str_to_check, str); > > const char *token = strtok_r (str_to_check, ",", &str_to_check); > > m_subset_list = riscv_cmdline_subset_list ()->clone (); > > @@ -247,8 +246,7 @@ riscv_process_one_target_attr (char *arg_str, > > return false; > > } > > > > - std::unique_ptr<char[]> buf (new char[len+1]); > > - char *str_to_check = buf.get(); > > + char *str_to_check = (char *) alloca (len + 1); > > strcpy (str_to_check, arg_str); > > > > char *arg = strchr (str_to_check, '='); > > @@ -334,8 +332,7 @@ riscv_process_target_attr (tree fndecl, tree args, location_t loc, > > return false; > > } > > > > - std::unique_ptr<char[]> buf (new char[len+1]); > > - char *str_to_check = buf.get (); > > + char *str_to_check = (char *) alloca (len + 1); > > strcpy (str_to_check, TREE_STRING_POINTER (args)); > > > > /* Used to catch empty spaces between commas i.e. > > -- > > 2.45.2 > >
On 7/9/24 7:12 AM, Christoph Müllner wrote: > On Tue, Jul 9, 2024 at 3:01 PM Kito Cheng <kito.cheng@sifive.com> wrote: >> >> IIRC Jeff mentions that it may introduce buffer overflow if the input >> string is long enough. > > The manpage states: > If the allocation causes stack overflow, program behavior is undefined. > > This was considered reasonable for AArch64: > https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/aarch64/aarch64.cc;h=7f0cc47d0f071de9297068baa85c6d5fc4d7fa5b;hb=HEAD#l19408 > So, my assumption was that it should be fine for RISC-V as well. > However, I won't insist on this patch. The key question is whether or not the length can potentially be attacker controlled. If so, then alloca is highly unsafe. An attacker can arrange to alloca a much larger region, with the goal of shifting the stack into the heap, or performing an under allocation due to integer overflow+wrapping. There's additional problems with alloca, but they're largely mitigated by stack clash which is typically on at the distro level, thankfully. My position is unless there is a highly compelling reason, we should just avoid alloca. And compelling reasons are few and far between. Jeff
Jeff Law <jeffreyalaw@gmail.com> writes: > On 7/9/24 7:12 AM, Christoph Müllner wrote: >> On Tue, Jul 9, 2024 at 3:01 PM Kito Cheng <kito.cheng@sifive.com> wrote: >>> >>> IIRC Jeff mentions that it may introduce buffer overflow if the input >>> string is long enough. >> >> The manpage states: >> If the allocation causes stack overflow, program behavior is undefined. >> >> This was considered reasonable for AArch64: >> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/aarch64/aarch64.cc;h=7f0cc47d0f071de9297068baa85c6d5fc4d7fa5b;hb=HEAD#l19408 >> So, my assumption was that it should be fine for RISC-V as well. >> However, I won't insist on this patch. > The key question is whether or not the length can potentially be > attacker controlled. If so, then alloca is highly unsafe. An attacker > can arrange to alloca a much larger region, with the goal of shifting > the stack into the heap, or performing an under allocation due to > integer overflow+wrapping. > > There's additional problems with alloca, but they're largely mitigated > by stack clash which is typically on at the distro level, thankfully. > > My position is unless there is a highly compelling reason, we should > just avoid alloca. And compelling reasons are few and far between. FWIW, I agree the use in aarch64 is a mistake. I'll send a patch later. Richard
diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc index 19eb7b06d548..e59cc53f23c6 100644 --- a/gcc/config/riscv/riscv-target-attr.cc +++ b/gcc/config/riscv/riscv-target-attr.cc @@ -109,8 +109,7 @@ riscv_target_attr_parser::parse_arch (const char *str) { /* Parsing the extension list like "+<ext>[,+<ext>]*". */ size_t len = strlen (str); - std::unique_ptr<char[]> buf (new char[len+1]); - char *str_to_check = buf.get (); + char *str_to_check = (char *) alloca (len + 1); strcpy (str_to_check, str); const char *token = strtok_r (str_to_check, ",", &str_to_check); m_subset_list = riscv_cmdline_subset_list ()->clone (); @@ -247,8 +246,7 @@ riscv_process_one_target_attr (char *arg_str, return false; } - std::unique_ptr<char[]> buf (new char[len+1]); - char *str_to_check = buf.get(); + char *str_to_check = (char *) alloca (len + 1); strcpy (str_to_check, arg_str); char *arg = strchr (str_to_check, '='); @@ -334,8 +332,7 @@ riscv_process_target_attr (tree fndecl, tree args, location_t loc, return false; } - std::unique_ptr<char[]> buf (new char[len+1]); - char *str_to_check = buf.get (); + char *str_to_check = (char *) alloca (len + 1); strcpy (str_to_check, TREE_STRING_POINTER (args)); /* Used to catch empty spaces between commas i.e.
Allocating an object on the heap with new, wrapping it in a std::unique_ptr and finally getting the buffer via buf.get() is a correct way to allocate a buffer that is automatically freed on return. However, a simple invocation of alloca() does the same with less overhead. gcc/ChangeLog: * config/riscv/riscv-target-attr.cc (riscv_target_attr_parser::parse_arch): Replace new + std::unique_ptr by alloca(). (riscv_process_one_target_attr): Likewise. (riscv_process_target_attr): Likewise. Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu> --- gcc/config/riscv/riscv-target-attr.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)