Message ID | 20190809063835.6717-2-ppandit@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | scsi: lsi: break infinite loop after 10k instructions | expand |
+-- 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
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 >
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
+-- 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
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
+-- 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
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