| Message ID | 20190808090652.2478-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068) | expand |
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. >
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. > >
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. >>>
+-- 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
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.