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

  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
  

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
  

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
>
  

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
  

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
>