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

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

[P J P]

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;
+    }
     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.
-- 
2.21.0

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

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

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

[Marcelo Tosatti]

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

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

[Philippe Mathieu-Daudé]

On 8/8/19 3:46 PM, Marcelo Tosatti wrote:
> 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.

Thanks for the explanation.
So we agree using DSTAT.IID is the correct thing to do.
Any volunteer to fix this? :)

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

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

[P J P]

+-- On Thu, 8 Aug 2019, Philippe Mathieu-Daudé wrote --+
| >>     trace_lsi_execute_script_tc_illegal();
| >>     lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
|
| So we agree using DSTAT.IID is the correct thing to do. Any volunteer to fix 
| this? :)

Sent patch v3. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F