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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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
>