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 <p...@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-sys...@rub.de>
>>> Signed-off-by: Prasad J Pandit <p...@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 <mtosa...@redhat.com>
>> Signed-off-by: Anthony Liguori <aligu...@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.
>>>
