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

[Philippe Mathieu-Daudé]

On 8/8/19 11:06 AM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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.

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":

$ 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 <address@hidden>
    Signed-off-by: Anthony Liguori <address@hidden>

$ 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.
>