[PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[Tsukasa OI]

Hello,

After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
instructions with .insn", I started to see some crashes while running
"make check-gas".

The cause was simple.  Some functions depended on the fact that maximum
length returned by riscv_insn_length is 8.  But since the commit above
increased that upper limit from 64-bits (8 bytes) to 176-bits (22 bytes),
we need to increase two buffer sizes to avoid crashes.

But note that this change doesn't really support over 64-bit instructions.
It can be said on riscv_insn::fetch_instruction because it now may return
only a part of instruction.
Instead of merging this, reverting that commit (for now) might be an option.

PATCH 1: Binutils
PATCH 2: GDB

Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Fix buffer overflow on print_insn_riscv
  gdb/riscv: Fix buffer overflow on riscv_insn::fetch_instruction

 gdb/riscv-tdep.c    | 2 +-
 opcodes/riscv-dis.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)


base-commit: 034235cebd790d4f9a1728043a175d7d7d9338b1
-- 
2.34.1

[PATCH 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_riscv): Increase buffer size for max
	176-bit length instructions.
---
 opcodes/riscv-dis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 6ac69490b78..66643431429 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -999,7 +999,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
 int
 print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
 {
-  bfd_byte packet[8];
+  bfd_byte packet[22];
   insn_t insn = 0;
   bfd_vma dump_size;
   int status;
-- 
2.34.1

[PATCH 2/2] gdb/riscv: Fix buffer overflow on riscv_insn::fetch_instruction

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

Note that this change will make the result of riscv_insn::fetch_instruction
partial when the instruction is longer than 64-bits.  To really support
instructions longer than 64-bit, we need something more.
---
 gdb/riscv-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 47d8f9e601b..99307bd2de1 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
   enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
-  gdb_byte buf[8];
+  gdb_byte buf[22];
   int instlen, status;
 
   /* All insns are at least 16 bits.  */
-- 
2.34.1

Re: [PATCH 2/2] gdb/riscv: Fix buffer overflow on riscv_insn::fetch_instruction

[Andreas Schwab]

On Okt 04 2022, Tsukasa OI via Gdb-patches wrote:

> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 47d8f9e601b..99307bd2de1 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
>  			       CORE_ADDR addr, int *len)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
> -  gdb_byte buf[8];
> +  gdb_byte buf[22];

Can the magic number be derived from something else so that is adapts
automatically?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[Jan Beulich]

On 04.10.2022 10:59, Tsukasa OI wrote:
> After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
> instructions with .insn", I started to see some crashes while running
> "make check-gas".

Hmm, I'm puzzled why things worked correctly for me. The extra size needed
is quite significant, so chances should be rather slim for things to work
correctly.

> The cause was simple.  Some functions depended on the fact that maximum
> length returned by riscv_insn_length is 8.  But since the commit above
> increased that upper limit from 64-bits (8 bytes) to 176-bits (22 bytes),
> we need to increase two buffer sizes to avoid crashes.
> 
> But note that this change doesn't really support over 64-bit instructions.
> It can be said on riscv_insn::fetch_instruction because it now may return
> only a part of instruction.
> Instead of merging this, reverting that commit (for now) might be an option.

Please let's try to avoid reverting - the ability to emit wide instructions
via .insn helps testsuites beyond binutils' / gas'es.

In any event - thanks for the quick fixing of the issue. I wonder though
whether a connection (at least by way of comments) should be established so
that the same oversight won't happen again (e.g. once the spec spells out
how even wider insns would be encoded).

Jan

Re: [PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[Tsukasa OI]

Jan,

On 2022/10/04 18:07, Jan Beulich wrote:
> On 04.10.2022 10:59, Tsukasa OI wrote:
>> After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
>> instructions with .insn", I started to see some crashes while running
>> "make check-gas".
> 
> Hmm, I'm puzzled why things worked correctly for me. The extra size needed
> is quite significant, so chances should be rather slim for things to work
> correctly.

I don't see this extra stack size as a problem so far.

> 
>> The cause was simple.  Some functions depended on the fact that maximum
>> length returned by riscv_insn_length is 8.  But since the commit above
>> increased that upper limit from 64-bits (8 bytes) to 176-bits (22 bytes),
>> we need to increase two buffer sizes to avoid crashes.
>>
>> But note that this change doesn't really support over 64-bit instructions.
>> It can be said on riscv_insn::fetch_instruction because it now may return
>> only a part of instruction.
>> Instead of merging this, reverting that commit (for now) might be an option.
> 
> Please let's try to avoid reverting - the ability to emit wide instructions
> via .insn helps testsuites beyond binutils' / gas'es.

I normally agree with you but the real problem is, most of the RISC-V
code is not yet ready for >64b instructions.

At least it breaks GDB (even with my PATCH v1).  I increased the buffer
size in riscv_insn::fetch_instruction but riscv_insn::decode (the caller
of fetch_instruction) causes an assertion failure.

>   else
>     {
>       /* This must be a 6 or 8 byte instruction, we don't currently decode
> 	 any of these, so just ignore it.  */
>       gdb_assert (m_length == 6 || m_length == 8);
>       m_opcode = OTHER;
>     }

So, I'll at least submit PATCH v2 based on your and Andreas' feedback to
patch minimally required portions but I will keep reverting as an
option.  With next PATCH v2, at least Binutils and GDB will be
"functional" again (so that reverting will be not that urgent).

Thanks,
Tsukasa

> 
> In any event - thanks for the quick fixing of the issue. I wonder though
> whether a connection (at least by way of comments) should be established so
> that the same oversight won't happen again (e.g. once the spec spells out
> how even wider insns would be encoded).
> 
> Jan
> 

Re: [PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[Jan Beulich]

On 04.10.2022 11:26, Tsukasa OI wrote:
> On 2022/10/04 18:07, Jan Beulich wrote:
>> On 04.10.2022 10:59, Tsukasa OI wrote:
>>> After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
>>> instructions with .insn", I started to see some crashes while running
>>> "make check-gas".
>>
>> Hmm, I'm puzzled why things worked correctly for me. The extra size needed
>> is quite significant, so chances should be rather slim for things to work
>> correctly.
> 
> I don't see this extra stack size as a problem so far.

I guess my wording was misleading: I would have expected things for me to
be broken as well, simply because the amount of overrun would have
clobbered multiple stack slots (the more that some of my testing was on a
32-bit host).

Jan

Re: [PATCH 0/2] RISC-V: Fix buffer overflow after long instruction support

[Tsukasa OI]

On 2022/10/04 18:44, Jan Beulich wrote:
> On 04.10.2022 11:26, Tsukasa OI wrote:
>> On 2022/10/04 18:07, Jan Beulich wrote:
>>> On 04.10.2022 10:59, Tsukasa OI wrote:
>>>> After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
>>>> instructions with .insn", I started to see some crashes while running
>>>> "make check-gas".
>>>
>>> Hmm, I'm puzzled why things worked correctly for me. The extra size needed
>>> is quite significant, so chances should be rather slim for things to work
>>> correctly.
>>
>> I don't see this extra stack size as a problem so far.
> 
> I guess my wording was misleading: I would have expected things for me to
> be broken as well, simply because the amount of overrun would have
> clobbered multiple stack slots (the more that some of my testing was on a
> 32-bit host).

Ah, that would make sense.

Thanks,
Tsukasa

> 
> Jan
> 

[PATCH v2 0/2] RISC-V: Fix buffer overflow after 176-bit instruction support

[Tsukasa OI]

Hello,

After commit bb996692bd9 "RISC-V/gas: allow generating up to 176-bit
instructions with .insn", I started to see some crashes while running
"make check-gas".

The cause was simple.  Some functions depended on the fact that maximum
length returned by riscv_insn_length is 8.  But since the commit above
increased that upper limit from 64-bits (8 bytes) to 176-bits (22 bytes),
we need to increase two buffer sizes to avoid crashes.

But note that this change doesn't really support over 64-bit instructions.
At least we can safely ignore those long instructions but we must remember
that we are still in a partial support for 176-bit instructions.

[Changes: v1 -> v2]
-   Fix assertion failure on riscv_insn::decode
-   Use new constant RISCV_MAX_INSN_LEN for buffer size

PATCH 1: Binutils
PATCH 2: GDB (v2: depends on PATCH 1)

Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Fix buffer overflow on print_insn_riscv
  gdb/riscv: Partial support for instructions up to 176-bits

 gdb/riscv-tdep.c       | 9 +++++----
 include/opcode/riscv.h | 2 ++
 opcodes/riscv-dis.c    | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)


base-commit: 034235cebd790d4f9a1728043a175d7d7d9338b1
-- 
2.34.1

[PATCH v2 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

include/ChangeLog:

	* opcode/riscv.h (RISCV_MAX_INSN_LEN): Max instruction length for
	use in buffer size.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_riscv): Increase buffer size for max
	176-bit length instructions.
---
 include/opcode/riscv.h | 2 ++
 opcodes/riscv-dis.c    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
index 9417dcf00c5..33415977bc7 100644
--- a/include/opcode/riscv.h
+++ b/include/opcode/riscv.h
@@ -55,6 +55,8 @@ static const char * const riscv_pred_succ[16] =
   "i", "iw", "ir", "irw", "io", "iow", "ior", "iorw"
 };
 
+#define RISCV_MAX_INSN_LEN 22  /* max 176-bit encoding.  */
+
 #define RVC_JUMP_BITS 11
 #define RVC_JUMP_REACH ((1ULL << RVC_JUMP_BITS) * RISCV_JUMP_ALIGN)
 
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 6ac69490b78..f5e5af3138c 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -999,7 +999,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
 int
 print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
 {
-  bfd_byte packet[8];
+  bfd_byte packet[RISCV_MAX_INSN_LEN];
   insn_t insn = 0;
   bfd_vma dump_size;
   int status;
-- 
2.34.1

Re: [PATCH v2 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Jan Beulich]

On 04.10.2022 11:45, Tsukasa OI wrote:
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -55,6 +55,8 @@ static const char * const riscv_pred_succ[16] =
>    "i", "iw", "ir", "irw", "io", "iow", "ior", "iorw"
>  };
>  
> +#define RISCV_MAX_INSN_LEN 22  /* max 176-bit encoding.  */

To be honest this still doesn't look sufficient to me: There's still
no connection between this constant and riscv_insn_length(). Yet both
want changing at the same time when it comes to insn length aspects.
As said in reply to v1 - comments may be one way of dealing with this.
We don't have BUILD_BUG_ON() or alike (and even if we had it wouldn't
be usable in a portable way), so an actual build time check might not
be feasible. A runtime check also doesn't look realistic, as

    gas_assert (riscv_insn_length(~0) == RISCV_MAX_INSN_LEN);

wouldn't be correct, and I'm unconvinced of using other than the most
simple ~0 as an argument here.

Jan

Re: [PATCH v2 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Tsukasa OI]

On 2022/10/04 18:58, Jan Beulich wrote:
> On 04.10.2022 11:45, Tsukasa OI wrote:
>> --- a/include/opcode/riscv.h
>> +++ b/include/opcode/riscv.h
>> @@ -55,6 +55,8 @@ static const char * const riscv_pred_succ[16] =
>>    "i", "iw", "ir", "irw", "io", "iow", "ior", "iorw"
>>  };
>>  
>> +#define RISCV_MAX_INSN_LEN 22  /* max 176-bit encoding.  */
> 
> To be honest this still doesn't look sufficient to me: There's still
> no connection between this constant and riscv_insn_length(). Yet both
> want changing at the same time when it comes to insn length aspects.
> As said in reply to v1 - comments may be one way of dealing with this.
> We don't have BUILD_BUG_ON() or alike (and even if we had it wouldn't
> be usable in a portable way), so an actual build time check might not
> be feasible. A runtime check also doesn't look realistic, as
> 
>     gas_assert (riscv_insn_length(~0) == RISCV_MAX_INSN_LEN);
> 
> wouldn't be correct, and I'm unconvinced of using other than the most
> simple ~0 as an argument here.
> 
> Jan
> 

I have to agree that the constant with no direct connection with
riscv_insn_length is not good but I don't come up with better solution
than this (with given constraints).
In any case, keeping this stack buffer overflow is definitely a bad idea
and we have to do something to deal with it in a days.

Tsukasa

Re: [PATCH v2 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Jan Beulich]

On 04.10.2022 12:13, Tsukasa OI wrote:
> On 2022/10/04 18:58, Jan Beulich wrote:
>> On 04.10.2022 11:45, Tsukasa OI wrote:
>>> --- a/include/opcode/riscv.h
>>> +++ b/include/opcode/riscv.h
>>> @@ -55,6 +55,8 @@ static const char * const riscv_pred_succ[16] =
>>>    "i", "iw", "ir", "irw", "io", "iow", "ior", "iorw"
>>>  };
>>>  
>>> +#define RISCV_MAX_INSN_LEN 22  /* max 176-bit encoding.  */
>>
>> To be honest this still doesn't look sufficient to me: There's still
>> no connection between this constant and riscv_insn_length(). Yet both
>> want changing at the same time when it comes to insn length aspects.
>> As said in reply to v1 - comments may be one way of dealing with this.
>> We don't have BUILD_BUG_ON() or alike (and even if we had it wouldn't
>> be usable in a portable way), so an actual build time check might not
>> be feasible. A runtime check also doesn't look realistic, as
>>
>>     gas_assert (riscv_insn_length(~0) == RISCV_MAX_INSN_LEN);
>>
>> wouldn't be correct, and I'm unconvinced of using other than the most
>> simple ~0 as an argument here.
> 
> I have to agree that the constant with no direct connection with
> riscv_insn_length is not good but I don't come up with better solution
> than this (with given constraints).
> In any case, keeping this stack buffer overflow is definitely a bad idea
> and we have to do something to deal with it in a days.

Agreed. Hence could you add cross-referencing comments at both sides
while introducing the #define, as a minimal measure?

Jan

Re: [PATCH v2 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Jan Beulich]

On 04.10.2022 12:16, Jan Beulich via Binutils wrote:
> On 04.10.2022 12:13, Tsukasa OI wrote:
>> On 2022/10/04 18:58, Jan Beulich wrote:
>>> On 04.10.2022 11:45, Tsukasa OI wrote:
>>>> --- a/include/opcode/riscv.h
>>>> +++ b/include/opcode/riscv.h
>>>> @@ -55,6 +55,8 @@ static const char * const riscv_pred_succ[16] =
>>>>    "i", "iw", "ir", "irw", "io", "iow", "ior", "iorw"
>>>>  };
>>>>  
>>>> +#define RISCV_MAX_INSN_LEN 22  /* max 176-bit encoding.  */
>>>
>>> To be honest this still doesn't look sufficient to me: There's still
>>> no connection between this constant and riscv_insn_length(). Yet both
>>> want changing at the same time when it comes to insn length aspects.
>>> As said in reply to v1 - comments may be one way of dealing with this.
>>> We don't have BUILD_BUG_ON() or alike (and even if we had it wouldn't
>>> be usable in a portable way), so an actual build time check might not
>>> be feasible. A runtime check also doesn't look realistic, as
>>>
>>>     gas_assert (riscv_insn_length(~0) == RISCV_MAX_INSN_LEN);
>>>
>>> wouldn't be correct, and I'm unconvinced of using other than the most
>>> simple ~0 as an argument here.
>>
>> I have to agree that the constant with no direct connection with
>> riscv_insn_length is not good but I don't come up with better solution
>> than this (with given constraints).
>> In any case, keeping this stack buffer overflow is definitely a bad idea
>> and we have to do something to deal with it in a days.
> 
> Agreed. Hence could you add cross-referencing comments at both sides
> while introducing the #define, as a minimal measure?

Or wait - why don't you move the #define _into_ riscv_insn_length(),
placed right at the position that would need touching when adding
support for wider insns (or when deciding to reduce support again)?

Jan

[PATCH v2 2/2] gdb/riscv: Partial support for instructions up to 176-bits

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

Also, that would break an assumption in riscv_insn::decode so this commit
fixes it.
---
 gdb/riscv-tdep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 47d8f9e601b..433311e1024 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
   enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
-  gdb_byte buf[8];
+  gdb_byte buf[RISCV_MAX_INSN_LEN];
   int instlen, status;
 
   /* All insns are at least 16 bits.  */
@@ -1933,9 +1933,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
     }
   else
     {
-      /* This must be a 6 or 8 byte instruction, we don't currently decode
-	 any of these, so just ignore it.  */
-      gdb_assert (m_length == 6 || m_length == 8);
+      /* 6 - 22 bytes instruction.  If the length is larger than 8, we don't
+	 have full instruction bits in ival.  At least, such long instructions
+	 are not defined yet, so just ignore it.  */
+      gdb_assert (m_length > 0 && m_length % 2 == 0);
       m_opcode = OTHER;
     }
 }
-- 
2.34.1

[PATCH v3 0/2] RISC-V: Fix buffer overflow after 176-bit instruction support

[Tsukasa OI]

Hello,

This is the PATCH v3 for quick stack buffer overflow fix.

[Changes: v2 -> v3]
-   PATCH 1/2: Define RISCV_MAX_INSN_LEN in riscv_insn_length function
    (idea by Jan Beulich)
-   PATCH 2/2: Fix comment and commit message

[Changes: v1 -> v2]
-   Fix assertion failure on riscv_insn::decode
-   Use new constant RISCV_MAX_INSN_LEN for buffer size

PATCH 1: Binutils
PATCH 2: GDB (v2/3: depends on PATCH 1)

Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: Fix buffer overflow on print_insn_riscv
  gdb/riscv: Partial support for instructions up to 176-bit

 gdb/riscv-tdep.c       | 9 +++++----
 include/opcode/riscv.h | 2 ++
 opcodes/riscv-dis.c    | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)


base-commit: d71eca64e70c31e0d32396a0b6c60e3ea9eb420b
-- 
2.34.1

[PATCH v3 1/2] RISC-V: Fix buffer overflow on print_insn_riscv

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase packet buffer size to 176-bit in size.

include/ChangeLog:

	* opcode/riscv.h (RISCV_MAX_INSN_LEN): Max instruction length for
	use in buffer size.

opcodes/ChangeLog:

	* riscv-dis.c (print_insn_riscv): Increase buffer size for max
	176-bit length instructions.
---
 include/opcode/riscv.h | 2 ++
 opcodes/riscv-dis.c    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
index 9417dcf00c5..b14f3d998b3 100644
--- a/include/opcode/riscv.h
+++ b/include/opcode/riscv.h
@@ -40,6 +40,8 @@ static inline unsigned int riscv_insn_length (insn_t insn)
   /* 80- ... 176-bit instructions.  */
   if ((insn & 0x7f) == 0x7f && (insn & 0x7000) != 0x7000)
     return 10 + ((insn >> 11) & 0xe);
+  /* Maximum value returned by this function.  */
+#define RISCV_MAX_INSN_LEN 22
   /* Longer instructions not supported at the moment.  */
   return 2;
 }
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 031c19334fa..2c0aed13e75 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -1000,7 +1000,7 @@ riscv_disassemble_data (bfd_vma memaddr ATTRIBUTE_UNUSED,
 int
 print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
 {
-  bfd_byte packet[8];
+  bfd_byte packet[RISCV_MAX_INSN_LEN];
   insn_t insn = 0;
   bfd_vma dump_size;
   int status;
-- 
2.34.1

[PATCH v3 2/2] gdb/riscv: Partial support for instructions up to 176-bit

[Tsukasa OI]

Because riscv_insn_length started to support instructions up to 176-bit,
we need to increase buf size to 176-bit in size.

Also, that would break an assumption in riscv_insn::decode so this commit
fixes it, noting that instructions longer than 64-bit are not fully
supported yet.
---
 gdb/riscv-tdep.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 47d8f9e601b..63ebed4ff19 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1770,7 +1770,7 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch,
 			       CORE_ADDR addr, int *len)
 {
   enum bfd_endian byte_order = gdbarch_byte_order_for_code (gdbarch);
-  gdb_byte buf[8];
+  gdb_byte buf[RISCV_MAX_INSN_LEN];
   int instlen, status;
 
   /* All insns are at least 16 bits.  */
@@ -1933,9 +1933,10 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc)
     }
   else
     {
-      /* This must be a 6 or 8 byte instruction, we don't currently decode
-	 any of these, so just ignore it.  */
-      gdb_assert (m_length == 6 || m_length == 8);
+      /* 6 bytes or more.  If the instruction is longer than 8 bytes, we don't
+	 have full instruction bits in ival.  At least, such long instructions
+	 are not defined yet, so just ignore it.  */
+      gdb_assert (m_length > 0 && m_length % 2 == 0);
       m_opcode = OTHER;
     }
 }
-- 
2.34.1

Re: [PATCH v3 0/2] RISC-V: Fix buffer overflow after 176-bit instruction support

[Jan Beulich]

On 04.10.2022 13:25, Tsukasa OI wrote:
> This is the PATCH v3 for quick stack buffer overflow fix.
> 
> [Changes: v2 -> v3]
> -   PATCH 1/2: Define RISCV_MAX_INSN_LEN in riscv_insn_length function
>     (idea by Jan Beulich)
> -   PATCH 2/2: Fix comment and commit message

Lgtm now, fwiw. Like elsewhere I'd prefer the arch maintainers to actually
approve the change to go in.

Jan

Re: [PATCH v3 0/2] RISC-V: Fix buffer overflow after 176-bit instruction support

[Nelson Chu]

OK, thanks.  If the problem is resolved and test cases are passed,
please commit.

Thanks
Nelson

On Tue, Oct 4, 2022 at 8:23 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 04.10.2022 13:25, Tsukasa OI wrote:
> > This is the PATCH v3 for quick stack buffer overflow fix.
> >
> > [Changes: v2 -> v3]
> > -   PATCH 1/2: Define RISCV_MAX_INSN_LEN in riscv_insn_length function
> >     (idea by Jan Beulich)
> > -   PATCH 2/2: Fix comment and commit message
>
> Lgtm now, fwiw. Like elsewhere I'd prefer the arch maintainers to actually
> approve the change to go in.
>
> Jan