From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Fam Zheng <fam@euphon.net>,
Prasad J Pandit <pjp@fedoraproject.org>,
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
QEMU Developers <qemu-devel@nongnu.org>,
P J P <ppandit@redhat.com>, Bugs SysSec <bugs-syssec@rub.de>,
Paolo Bonzini <pbonzini@redhat.com>,
Stefano Garzarella <sgarzare@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068)
Date: Thu, 8 Aug 2019 15:59:36 +0200 [thread overview]
Message-ID: <84440ce1-374d-e612-c7a9-184e55242ffe@redhat.com> (raw)
In-Reply-To: <20190808134646.GA6915@amt.cnet>
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.
>>>
next prev parent reply other threads:[~2019-08-08 14:00 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-08 9:06 [Qemu-devel] [PATCH v2] scsi: lsi: exit infinite loop while executing script (CVE-2019-12068) P J P
2019-08-08 9:31 ` Philippe Mathieu-Daudé
2019-08-08 13:46 ` Marcelo Tosatti
2019-08-08 13:59 ` Philippe Mathieu-Daudé [this message]
2019-08-09 6:42 ` P J P
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84440ce1-374d-e612-c7a9-184e55242ffe@redhat.com \
--to=philmd@redhat.com \
--cc=bugs-syssec@rub.de \
--cc=fam@euphon.net \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=mtosatti@redhat.com \
--cc=pbonzini@redhat.com \
--cc=pjp@fedoraproject.org \
--cc=ppandit@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=sgarzare@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).