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

[Thomas Huth]

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?

 Thomas