Re: [Qemu-devel] [PATCH v3 1/2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)
+-- 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
Re: [Qemu-devel] [PATCH v3 1/2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)
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_INSN1
> | +
> | 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)
+-- 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_INSN1
| +
| 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)
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_INSN1
+
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 > 1 && 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 &= 0x00ff;
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)
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
> |
> | 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
> | Signed-off-by: Prasad J Pandit
> | ---
> | 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_INSN1
> | +
> | 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)
+-- On Fri, 9 Aug 2019, P J P wrote --+
| From: Prasad J Pandit
|
| 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
| Signed-off-by: Prasad J Pandit
| ---
| 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_INSN1
| +
| 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
