On 25/10/2018 22:09, P J P wrote:
> From: Prasad J Pandit <p...@fedoraproject.org>
>
> While writing a message in 'lsi_do_msgin', message length value
> in msg_len could be invalid, add check to avoid OOB access issue.
>
> Reported-by: Ameya More <ameya.m...@oracle.com>
> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> ---
> hw/scsi/lsi53c895a.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..a266c5a113 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -205,7 +205,7 @@ typedef struct {
> /* Action to take at the end of a MSG IN phase.
> 0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN. */
> int msg_action;
> - 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.
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.
> uint8_t msg[LSI_MAX_MSGIN_LEN];
> /* 0 if SCRIPTS are running or stopped.
> * 1 if a Wait Reselect instruction has been issued.
> @@ -861,12 +861,15 @@ 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;
> if (len > s->dbc)
> len = s->dbc;
> + if (len > LSI_MAX_MSGIN_LEN) {
> + len = LSI_MAX_MSGIN_LEN;
> + }
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.
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.
Thanks,
Paolo
> 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];
> @@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
> VMSTATE_INT32(carry, LSIState),
> VMSTATE_INT32(status, LSIState),
> VMSTATE_INT32(msg_action, LSIState),
> - VMSTATE_INT32(msg_len, LSIState),
> + VMSTATE_UINT8(msg_len, LSIState),
> VMSTATE_BUFFER(msg, LSIState),
> VMSTATE_INT32(waiting, LSIState),
>
>
