Re: [Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[Paolo Bonzini]

On 29/10/2018 18:56, Paolo Bonzini wrote:
> On 26/10/2018 22:55, Peter Maydell wrote:
>>> +    assert(len <= LSI_MAX_MSGIN_LEN);
>>>      pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
>>>      /* Linux drivers rely on the last byte being in the SIDL.  */
>>>      s->sidl = s->msg[len - 1];
>> Is it possible to get here with len == 0 ?
> 
> No, all calls to
> 
>     lsi_set_phase(s, PHASE_MI);
> 
> are followed or preceded by lsi_add_msg_byte.  But an assertion is good
> to add.  What do you think of squashing this on top:
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 3a40e62853..72d85c42dd 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -865,9 +865,9 @@ static void lsi_do_msgin(LSIState *s)
>      trace_lsi_do_msgin(s->dbc, s->msg_len);
>      s->sfbr = s->msg[0];
>      len = s->msg_len;
> +    assert(len >= 0 && len <= LSI_MAX_MSGIN_LEN);

Ahem, len > 0.  Is there a CVE number?

Paolo

>      if (len > s->dbc)
>          len = s->dbc;
> -    assert(len <= LSI_MAX_MSGIN_LEN);
>      pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
>      /* Linux drivers rely on the last byte being in the SIDL.  */
>      s->sidl = s->msg[len - 1];
> @@ -1706,8 +1706,10 @@
>          break;
>      case 0x58: /* SBDL */
>          /* Some drivers peek at the data bus during the MSG IN phase.  */
> -        if ((s->sstat1 & PHASE_MASK) == PHASE_MI)
> +        if ((s->sstat1 & PHASE_MASK) == PHASE_MI) {
> +            assert(s->msg_len >= 0);
>              return s->msg[0];
> +        }
>          ret = 0;
>          break;
>      case 0x59: /* SBDL high */
> 
> 
> Paolo
>