Re: [PATCH] hw/scsi/lsi53c895a: Fix reentrancy issues in the LSI controller (CVE-2023-0330)

[Stefan Hajnoczi]

On Tue, 16 May 2023 at 10:50, Thomas Huth <thuth@redhat.com> wrote:
>
> On 16/05/2023 15.40, Stefan Hajnoczi wrote:
> > On Tue, 16 May 2023 at 06:20, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >> We cannot use the generic reentrancy guard in the LSI code, so
> >> we have to manually prevent endless reentrancy here. The problematic
> >> lsi_execute_script() function has already a way to detect whether
> >> too many instructions have been executed - we just have to slightly
> >> change the logic here that it also takes into account if the function
> >> has been called too often in a reentrant way.
> >>
> >> The code in fuzz-lsi53c895a-test.c has been taken from an earlier
> >> patch by Mauro Matteo Cascella.
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1563
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   hw/scsi/lsi53c895a.c               |  7 ++++++-
> >>   tests/qtest/fuzz-lsi53c895a-test.c | 33 ++++++++++++++++++++++++++++++
> >>   2 files changed, 39 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> >> index 048436352b..8b34199ab8 100644
> >> --- a/hw/scsi/lsi53c895a.c
> >> +++ b/hw/scsi/lsi53c895a.c
> >> @@ -1134,10 +1134,13 @@ static void lsi_execute_script(LSIState *s)
> >>       uint32_t addr, addr_high;
> >>       int opcode;
> >>       int insn_processed = 0;
> >> +    static int reentrancy_level;
> >> +
> >> +    reentrancy_level++;
> >>
> >>       s->istat1 |= LSI_ISTAT1_SRUN;
> >>   again:
> >> -    if (++insn_processed > LSI_MAX_INSN) {
> >> +    if (++insn_processed > LSI_MAX_INSN || reentrancy_level > 8) {
> >
> > Why 8 and not some other number?
>
> It's just a random number which seemed to be a good compromise to me. We
> need at least 2 to be able to boot Linux. And if I add some debug printf and
> use it with the reproducer from the bug ticket, QEMU crashes after it
> reached level 1569. Anything in between should be OK at a first glance, but
> I'd take at least 3 or 4 to be one the safe side for some exotic use cases.
> 8 should really cover all real life cases, I guess. Or what would you suggest?

Please add a comment explaining that 8 is an arbitrary number that
should be sufficient for all legitimate guest drivers.

Thanks,
Stefan