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