Re: [Qemu-devel] [PATCH v2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Aug 08, 2019 at 11:31:02AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/8/19 11:06 AM, P J P wrote:
> > From: Prasad J Pandit <pjp@fedoraproject.org>
> > 
> > When executing script in lsi_execute_script(), the LSI scsi
> > adapter emulator advances 's->dsp' index to read next opcode.
> > This can lead to an infinite loop if the next opcode is empty.
> > Exit such loop after reading 10k empty opcodes.
> > 
> > Reported-by: Bugs SysSec <bugs-syssec@rub.de>
> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> > ---
> >  hw/scsi/lsi53c895a.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > Update v2: define LSI_MAX_INSN 10000
> >   -> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01370.html
> > 
> > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> > index 10468c1ec1..2adab341b1 100644
> > --- a/hw/scsi/lsi53c895a.c
> > +++ b/hw/scsi/lsi53c895a.c
> > @@ -185,6 +185,9 @@ static const char *names[] = {
> >  /* Flag set if this is a tagged command.  */
> >  #define LSI_TAG_VALID     (1 << 16)
> >  
> > +/* Maximum instructions to process. */
> > +#define LSI_MAX_INSN    10000
> > +
> >  typedef struct lsi_request {
> >      SCSIRequest *req;
> >      uint32_t tag;
> > @@ -1132,7 +1135,10 @@ static void lsi_execute_script(LSIState *s)
> >  
> >      s->istat1 |= LSI_ISTAT1_SRUN;
> >  again:
> > -    insn_processed++;
> > +    if (++insn_processed > LSI_MAX_INSN) {
> > +        s->waiting = LSI_NOWAIT;
> > +        goto exitloop;
> > +    }
> 
> If I understand the datasheet correctly, the model should set the
> DSTAT.IID bit.
> 
>   Illegal Instruction Detected
> 
>   This status bit is set any time an illegal or reserved
>   instruction opcode is detected, whether the LSI53C895A
>   is operating in single step mode or automatically
>   executing SCSI SCRIPTS.

Sounds the correct thing to do (exiting the loop seems arbitrary). 

> We already have:
> 
>     trace_lsi_execute_script_tc_illegal();
>     lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
> 
> Cc'ing Marcelo Tosatti since it is hard to understand the "Windows SCSI
> driver hack":

What this patch is, if an infinite loop is detected, to raise UDC
exception (Unexpected Disconnect). This would cause the driver to 
restart processing, which would work around the infinite loop problem.

> $ git show ee4d919f30f
> commit ee4d919f30f1378cda697dd94d5a21b2a7f4d90d
> Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Mon Sep 22 16:04:16 2008 +0000
> 
>     LSI SCSI: raise UDC on infinite loop (Marcelo Tosatti)
> 
>     Raise UDC (Unexpected Disconnect) when a large enough number of
>     instructions has been executed by the SCRIPTS processor. This "solution"
>     is much simpler than temporarily interrupting execution.
> 
>     This remedies the situation with Windows which downloads SCRIPTS code
>     that busy loops on guest main memory. Their drivers _do_ handle UDC
>     appropriately (at least XP and 2003).
> 
>     It would be nicer to actually detect infinite loops, but until then,
>     this bandaid seems acceptable.
> 
>     Since the situation seems to be rare enough, raise the number
>     of instructions to 10000 (previously 1000).
> 
>     Three people other than myself had success with this patch.
> 
>     Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> $ git show 64c68080da4
> commit 64c68080da429edf30a9857e3a698cb9ed335bd3
> Author: pbrook <pbrook@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Mon Sep 22 16:30:29 2008 +0000
> 
>     Add comment to windows SCSI hack.
> 
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index e45eefaef7..53a2add0df 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -1199,6 +1199,11 @@ again:
>          }
>      }
>      if (insn_processed > 10000 && !s->waiting) {
> +        /* Some windows drivers make the device spin waiting for a memory
> +           location to change.  If we have been executed a lot of code then
> +           assume this is the case and force an unexpected device
> disconnect.
> +           This is apparently sufficient to beat the drivers into
> submission.
> +         */
>          if (!(s->sien0 & LSI_SIST0_UDC))
>              fprintf(stderr, "inf. loop with UDC masked\n");
>          lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
> 
> >      insn = read_dword(s, s->dsp);
> >      if (!insn) {
> >          /* If we receive an empty opcode increment the DSP by 4 bytes
> > @@ -1569,7 +1575,8 @@ again:
> >              }
> >          }
> >      }
> > -    if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) {
> > +exitloop:
> > +    if (insn_processed > LSI_MAX_INSN && s->waiting == LSI_NOWAIT) {
> >          /* Some windows drivers make the device spin waiting for a memory
> >             location to change.  If we have been executed a lot of code then
> >             assume this is the case and force an unexpected device disconnect.
> >