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

[Paolo Bonzini]

On 30/10/2018 07:28, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> While writing a message in 'lsi_do_msgin', message length value
> in 'msg_len' could be invalid. Add check to avoid OOB access issue.
> 
> Signed-off-by: Prasad J Pandit <address@hidden>

Reviewed-by: Paolo Bonzini <address@hidden>

with one change below:

> ---
>  hw/scsi/lsi53c895a.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> Update v2: modify assert()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg06273.html
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..dc39f4c3ee 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -861,10 +861,11 @@ static void lsi_do_status(LSIState *s)
>  
>  static void lsi_do_msgin(LSIState *s)
>  {
> -    int len;
> +    uint8_t len;
>      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);
>      if (len > s->dbc)
>          len = s->dbc;
>      pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
> @@ -1705,8 +1706,10 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
>          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);

should be > 0 as well.

Paolo

>              return s->msg[0];
> +        }
>          ret = 0;
>          break;
>      case 0x59: /* SBDL high */
> @@ -2103,11 +2106,23 @@ static int lsi_pre_save(void *opaque)
>      return 0;
>  }
>  
> +static int lsi_post_load(void *opaque, int version_id)
> +{
> +    LSIState *s = opaque;
> +
> +    if (s->msg_len < 0 || s->msg_len > LSI_MAX_MSGIN_LEN) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_lsi_scsi = {
>      .name = "lsiscsi",
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .pre_save = lsi_pre_save,
> +    .post_load = lsi_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(parent_obj, LSIState),
>  
>