[Qemu-devel] [PATCH v3 0/2] scsi: lsi: break infinite loop after 10k instructions

[Qemu-devel] [PATCH v3 0/2] scsi: lsi: break infinite loop after 10k instructions

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

While executing script, the LSI SCSI Adapter emulator could run into an
infinite loop, if next instruction read by 's->dsp' index has an empty
opcode. Raise an illegal instruction interrupt and exit the loop after
10k iterations.

 -> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html

Thank you.
--
Prasad J Pandit (2):
  scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)
  scsi: lsi: use macro LSI_MAX_INSN instead of a magic number

 hw/scsi/lsi53c895a.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/2] 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, 10 insertions(+), 1 deletion(-)

Update v3: raise an illegal instruction interrupt and exit
  -> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 10468c1ec1..e703ef4c9d 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,13 @@ static void lsi_execute_script(LSIState *s)
 
     s->istat1 |= LSI_ISTAT1_SRUN;
 again:
-    insn_processed++;
+    if (++insn_processed > LSI_MAX_INSN) {
+        trace_lsi_execute_script_tc_illegal();
+        lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
+        lsi_disconnect(s);
+        trace_lsi_execute_script_stop();
+        return;
+    }
     insn = read_dword(s, s->dsp);
     if (!insn) {
         /* If we receive an empty opcode increment the DSP by 4 bytes
-- 
2.21.0

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

[P J P]

+-- On Fri, 9 Aug 2019, 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, 10 insertions(+), 1 deletion(-)
| 
| Update v3: raise an illegal instruction interrupt and exit
|   -> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html
| 
| diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
| index 10468c1ec1..e703ef4c9d 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,13 @@ static void lsi_execute_script(LSIState *s)
|  
|      s->istat1 |= LSI_ISTAT1_SRUN;
|  again:
| -    insn_processed++;
| +    if (++insn_processed > LSI_MAX_INSN) {
| +        trace_lsi_execute_script_tc_illegal();
| +        lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
| +        lsi_disconnect(s);
| +        trace_lsi_execute_script_stop();
| +        return;
| +    }
|      insn = read_dword(s, s->dsp);
|      if (!insn) {
|          /* If we receive an empty opcode increment the DSP by 4 bytes
| 

Ping...?!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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

[Philippe Mathieu-Daudé]

Hi Prasad,

On 8/13/19 12:05 PM, P J P wrote:
> +-- On Fri, 9 Aug 2019, 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, 10 insertions(+), 1 deletion(-)
> | 
> | Update v3: raise an illegal instruction interrupt and exit
> |   -> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html
> | 
> | diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> | index 10468c1ec1..e703ef4c9d 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,13 @@ static void lsi_execute_script(LSIState *s)
> |  
> |      s->istat1 |= LSI_ISTAT1_SRUN;
> |  again:
> | -    insn_processed++;
> | +    if (++insn_processed > LSI_MAX_INSN) {
> | +        trace_lsi_execute_script_tc_illegal();
> | +        lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
> | +        lsi_disconnect(s);
> | +        trace_lsi_execute_script_stop();
> | +        return;

My understanding of Marcelo's explanation
(https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html) is
we can kill insn_processed.

> | +    }
> |      insn = read_dword(s, s->dsp);
> |      if (!insn) {
> |          /* If we receive an empty opcode increment the DSP by 4 bytes
> | 
> 
> Ping...?!
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
> 

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

[Paolo Bonzini]

On 13/08/19 12:31, Philippe Mathieu-Daudé wrote:
>> |  
>> |      s->istat1 |= LSI_ISTAT1_SRUN;
>> |  again:
>> | -    insn_processed++;
>> | +    if (++insn_processed > LSI_MAX_INSN) {
>> | +        trace_lsi_execute_script_tc_illegal();
>> | +        lsi_script_dma_interrupt(s, LSI_DSTAT_IID);
>> | +        lsi_disconnect(s);
>> | +        trace_lsi_execute_script_stop();
>> | +        return;
> My understanding of Marcelo's explanation
> (https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01427.html) is
> we can kill insn_processed.
> 

All zeros is not an illegal instruction, it's just a block move with 
zero transfer count.  It's not clear to me from the spec that the 
behavior of QEMU, skipping the second word, is correct, but I do not 
really dare changing it.

After the first instruction is processed, "again" is only reached if 
s->waiting == LSI_NOWAIT.  Therefore, we could move the Windows hack to 
the beginning and remove the s->waiting condition.  The only change 
would be that it would also be triggered by all zero instructions, like this:

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 10468c1..9d714af 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,19 @@ static void lsi_execute_script(LSIState *s)
 
     s->istat1 |= LSI_ISTAT1_SRUN;
 again:
-    insn_processed++;
+    if (++insn_processed > LSI_MAX_INSN) {
+        /* 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)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "lsi_scsi: inf. loop with UDC masked");
+        }
+        lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
+        lsi_disconnect(s);
+    }
     insn = read_dword(s, s->dsp);
     if (!insn) {
         /* If we receive an empty opcode increment the DSP by 4 bytes
@@ -1569,19 +1584,7 @@ again:
             }
         }
     }
-    if (insn_processed > 10000 && 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.
-           This is apparently sufficient to beat the drivers into submission.
-         */
-        if (!(s->sien0 & LSI_SIST0_UDC)) {
-            qemu_log_mask(LOG_GUEST_ERROR,
-                          "lsi_scsi: inf. loop with UDC masked");
-        }
-        lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
-        lsi_disconnect(s);
-    } else if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) {
+    if (s->istat1 & LSI_ISTAT1_SRUN && s->waiting == LSI_NOWAIT) {
         if (s->dcntl & LSI_DCNTL_SSM) {
             lsi_script_dma_interrupt(s, LSI_DSTAT_SSI);
         } else {
@@ -1969,6 +1972,10 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
     case 0x2f: /* DSP[24:31] */
         s->dsp &= 0x00ffffff;
         s->dsp |= val << 24;
+        /*
+         * FIXME: if s->waiting != LSI_NOWAIT, this will only execute one
+         * instruction.  Is this correct?
+         */
         if ((s->dmode & LSI_DMODE_MAN) == 0
             && (s->istat1 & LSI_ISTAT1_SRUN) == 0)
             lsi_execute_script(s);
@@ -1987,6 +1994,10 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)
         break;
     case 0x3b: /* DCNTL */
         s->dcntl = val & ~(LSI_DCNTL_PFF | LSI_DCNTL_STD);
+        /*
+         * FIXME: if s->waiting != LSI_NOWAIT, this will only execute one
+         * instruction.  Is this correct?
+         */
         if ((val & LSI_DCNTL_STD) && (s->istat1 & LSI_ISTAT1_SRUN) == 0)
             lsi_execute_script(s);
         break;

Does it make sense?  Do you have a reproducer, and does the above
patch work?  Also, can the reproducer be modified into a qtest test
case?

Thanks,

Paolo

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

[P J P]

+-- On Tue, 13 Aug 2019, Paolo Bonzini wrote --+
| After the first instruction is processed, "again" is only reached if 
| s->waiting == LSI_NOWAIT.  Therefore, we could move the Windows hack to the 
| beginning and remove the s->waiting condition.  The only change would be 
| that it would also be triggered by all zero instructions, like this:
| 
| diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
| index 10468c1..9d714af 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,19 @@ static void lsi_execute_script(LSIState *s)
|  
|      s->istat1 |= LSI_ISTAT1_SRUN;
|  again:
| -    insn_processed++;
| +    if (++insn_processed > LSI_MAX_INSN) {
| +        /* 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)) {
| +            qemu_log_mask(LOG_GUEST_ERROR,
| +                          "lsi_scsi: inf. loop with UDC masked");
| +        }
| +        lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
| +        lsi_disconnect(s);
...
|
| Does it make sense? 

Yes, this'd also work, but need to return after lsi_disconnect(s), otherwise 
loop would continue.

Should I send a revised patch? (with above change)

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

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

[Paolo Bonzini]

On 14/08/19 12:25, P J P wrote:
> +-- On Tue, 13 Aug 2019, Paolo Bonzini wrote --+
> | After the first instruction is processed, "again" is only reached if 
> | s->waiting == LSI_NOWAIT.  Therefore, we could move the Windows hack to the 
> | beginning and remove the s->waiting condition.  The only change would be 
> | that it would also be triggered by all zero instructions, like this:
> | 
> | diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> | index 10468c1..9d714af 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,19 @@ static void lsi_execute_script(LSIState *s)
> |  
> |      s->istat1 |= LSI_ISTAT1_SRUN;
> |  again:
> | -    insn_processed++;
> | +    if (++insn_processed > LSI_MAX_INSN) {
> | +        /* 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)) {
> | +            qemu_log_mask(LOG_GUEST_ERROR,
> | +                          "lsi_scsi: inf. loop with UDC masked");
> | +        }
> | +        lsi_script_scsi_interrupt(s, LSI_SIST0_UDC, 0);
> | +        lsi_disconnect(s);
> ...
> |
> | Does it make sense? 
> 
> Yes, this'd also work, but need to return after lsi_disconnect(s), otherwise 
> loop would continue.
> 
> Should I send a revised patch? (with above change)

Yes, please.

Paolo

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

[P J P]

+-- On Wed, 14 Aug 2019, Paolo Bonzini wrote --+
| On 14/08/19 12:25, P J P wrote:
| > Should I send a revised patch? (with above change)
| 
| Yes, please.

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

[Qemu-devel] [PATCH v3 2/2] scsi: lsi: use macro LSI_MAX_INSN instead of a magic number

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Use macro 'LSI_MAX_INSN' instead of a magic number 10000.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/lsi53c895a.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index e703ef4c9d..f6786607f8 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -1578,7 +1578,7 @@ again:
             }
         }
     }
-    if (insn_processed > 10000 && s->waiting == LSI_NOWAIT) {
+    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