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

[P J P]

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| > -    int msg_len;
| > +    uint8_t msg_len;
| 
| Not wrong per se, but it's also not clear why it's needed.  I understand
| that you want to switch from signed to unsigned, but it is not mentioned
| in the commit message.

Changed to uint8_t because IIUC 'msg_len' is likely be < LSI_MAX_MSGIN_LEN=8. 
And uint32_t seems rather large for it.
 
| The switch to 8-bit, and below from VMSTATE_INT32 to VMSTATE_UINT8, is
| wrong because it changes the format of the live migration stream.

I see.

| I'm not sure it's appropriate to check for out of bounds reads here,
| because if s->msg_len is greater than LSI_MAX_MSGIN_LEN, then this test
| doesn't exclude you've already had an out of bounds write before.
| Indeed the msg_len is checked in lsi_add_msg_byte in order to avoid out
| of bounds accesses in either lsi_add_msg_byte or lsi_do_msgin. You
| could assert here that the variable is in range, I guess.

Okay.

| However, the out of bounds s->msg_len can actually happen in one other
| case: namely, if a malicious live migration stream includes a bogus
| s->msg_len.  Such live migration stream should be rejected; the fix for
| that is to add a lsi_post_load function, point to it in
| vmstate_lsi_scsi, and check there for s->msg_len <= LSI_MAX_MSGIN_LEN.

Okay, sending a revised patch v1 in a bit.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F