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

From: P J P <ppandit@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Fam Zheng" <fam@euphon.net>,
	"Marcelo Tosatti" <mtosatti@redhat.com>,
	"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Bugs SysSec" <bugs-syssec@rub.de>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Stefano Garzarella" <sgarzare@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v3 1/2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)
Date: Wed, 14 Aug 2019 15:55:41 +0530 (IST)	[thread overview]
Message-ID: <nycvar.YSQ.7.76.1908141450520.30613@xnncv> (raw)
In-Reply-To: <3a48e557-985d-7274-81d9-fe963f74e59a@redhat.com>

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